|
It used to be (before
Attached is my first attempt at fixing this issue. No tests yet. It logs the exception when it tries to turn on tracing when establishing the connection. The problem is we get this exception for every connection, which seems like a lot of output. Before
Any thoughts on how to handle this better? Well, it seems better with a lot of output than silently doing nothing, so it seems an improvement to me..
Trace was requested, after all. I think the directory should be created if permissions are in place, though. Do you think we should attempt to create the trace directory, even for release branches or make that change only on the trunk?
Attached is a ptch derby-3701_diff.txt which resolves this issue on trunk. We will now attempt to create the trace directory if it does not exist and will no longer hang if the trace file cannot be accessed. I added tests to NetworkServerControlApiTest. Running tests now.
I keep going back and forth on your question. I think - as Dag said on the list - if the permissions are in place we should create the dirs.
But should this be backported? Or should we just give an error message in those earlier versions? Either way will change the behavior, and might be an unwelcome surprise to people who have set this in their application. But of course not having a connection is worse, that does need to get fixed. I think maybe a compromise would be to have the stack print to derby.log in older versions. It would still mean more disk space used but not as much as if the full tracing would get created. +1 to Myrna's compromise, creating the directory is new behavior, which would presumably necessitate a change in the docs as well, not just a release note, so better reserve that for trunk.
Before I check this into the trunk, I would appreciate review. The exception handling is kind of messy since we need to throw an exception when NetworkServerControl.trace(true) fails but only log an exception if tracing fails on making a connection. I did this by adding a parameter to Session.initTrace to determine whether to throw the exception or not. I'd appreciate feedback on whether that's ok.
Kathey, i went through the patch and just have couple comments.
For DssTrace.java, there is following code in the diff file, Exception e = pae.getException(); if (e instanceof SecurityException) - throw (SecurityException)pae.getException(); + throw (SecurityException) e; else - throw (IOException) pae.getException(); + throw (IOException) e; Do we need to know what kind of exception it is before we throw it? In other words, could we just do? Exception e = pae.getException(); + throw e; - if (e instanceof SecurityException) - throw (SecurityException)pae.getException(); - else - throw (IOException) pae.getException(); Also, I wonder if it will be good to test that /trace/Server1.trace does not exist after making a Connection in xtestTraceSystemPropertiesNoPermission test. Other than that, the patch looks good to me. Thanks Mamta for looking at the patch. I'll make the change in DssTrace.
For the test we don't have permission to read that directory so can't tell whether it created the file or not unfortunately. Kathey Attached is a release note for this issue. I intend to backport the change, less the directory creation to 10.4 and 10.3.
Maybe it would be good to say that any intervening directories in the given path will be created as well (I tried and they did on Solaris anyway). This may not be obvious to some users, cf. the -p option in many Unix mkdir(1) commands.
Is the exists check necessary in the code below? In Sun's class library at least, File.mkdirs() first calls File.exists() and does nothing if the directory exists. Since mkdirs() doesn't throw IOException, I believe it should be safe in other JVMs as well.
+ if (!PrivilegedFileOps.exists(traceDirectory)) + { + PrivilegedFileOps.mkdirs(traceDirectory); + } mkdirs()'s javadoc has an invalid HTML tag (<code> true </true>). Fixed in revision 664524. The indentation in Session.java seems to be off. Probably a wrong tab size setting in your IDE. Fixed in revision 664530. Attaching updated releaseNote to indicate that intervening directories will also be created (per Dag's suggestion).
Changed an uppercase begin-bold tag to lowercase so that it would match the corresponding lowercase end-bold tag and pass the lint tool for release notes: org.apache.derbyBuild.ReleaseNoteReader
Hi Kathey,
Knut was wondering on If I understand correctly, the backport to older branches still represents a change from earlier behavior of older releases off those branches in that now a warning/error message will come to derby.log (although the dirs still do not get created).
The one reason why I think we might want to have a release note mentioning that is that derby.log can get really big that way. Thanks for the explanation, Myrna. I'm inlined to include this release note in the 10.4.2 issues list. Attaching a new version of releaseNote.html. This version clarifies the distinction between branches and releases.
Thanks, the changes (and the explanation) made it much clearer to me.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
java.lang.Exception: DRDA_UnableToAccept.S:Unable to accept connections and client hang if tracing is turned on but traceDirectory does not exist
DERBY-3110