Derby
  1. Derby
  2. DERBY-3618

Perform thread dump with ASSERTS with jdk 1.5 or higher

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.5.1.1
    • Fix Version/s: 10.5.1.1
    • Component/s: Services
    • Labels:
      None

      Description

      It would be good to have a stack traces for all threads dump to the derby.log when an assertion occurs with JVM's that support it.

      1. DERBY-3618_1.diff
        7 kB
        Erlend Birkenes
      2. DERBY-3618_2.diff
        7 kB
        Erlend Birkenes
      3. DERBY-3618_3.diff
        21 kB
        Erlend Birkenes
      4. DERBY-3618_4.diff
        22 kB
        Erlend Birkenes
      5. DERBY-3618_5.diff
        22 kB
        Erlend Birkenes
      6. DERBY-3618_6.diff
        24 kB
        Erlend Birkenes
      7. DERBY-3618_7.diff
        27 kB
        Erlend Birkenes
      8. DERBY-3618_8.diff
        16 kB
        Erlend Birkenes

        Issue Links

          Activity

          Kathey Marsden created issue -
          Erlend Birkenes made changes -
          Field Original Value New Value
          Assignee Erlend Birkenes [ ebirkenes ]
          Hide
          Kathey Marsden added a comment -

          Thanks Erlend for picking up this issue. You might want to look at the code for org.apache.derbyTesting.functionTests.util.ThreadDump and org.apache.derbyTesting.functionTests.util.TestUtil.dumpAllStackTracesIfSupported for sample code on how to dump the stack traces. Something similar will need to be brought into org.apache.derby.shared.common.sanity.SanityManager.THROWASSERT for this issue.

          The testing classes are not available from the code.

          Kathey

          Show
          Kathey Marsden added a comment - Thanks Erlend for picking up this issue. You might want to look at the code for org.apache.derbyTesting.functionTests.util.ThreadDump and org.apache.derbyTesting.functionTests.util.TestUtil.dumpAllStackTracesIfSupported for sample code on how to dump the stack traces. Something similar will need to be brought into org.apache.derby.shared.common.sanity.SanityManager.THROWASSERT for this issue. The testing classes are not available from the code. Kathey
          Kathey Marsden made changes -
          Link This issue is related to DERBY-666 [ DERBY-666 ]
          Hide
          Kathey Marsden added a comment -

          DERBY-666 is another case where it would be good to dump stack traces if permissions allow.

          Show
          Kathey Marsden added a comment - DERBY-666 is another case where it would be good to dump stack traces if permissions allow.
          Hide
          Kathey Marsden added a comment -

          A couple of things to consider when working on this issue ...
          1) This will require more permissons in the testing policy file for derby.jar. It would be nice if they could be added just for sane builds but I don't think that is possible.

          The two permissions that will be needed to dump the thread stack traces are:
          permission java.lang.RuntimePermission "getStackTrace";
          permission java.lang.RuntimePermission "modifyThreadGroup";

          The change should be as such that if these permissions are not present it will just skip the thread dump.

          2) Testing might be tricky, since we don't really have JUnit unit testing for internal methods. Maybe it will be easy enough to add if the methods are public. Also the output goes to derby.log which is hard to check. We may just have to settle for manual testing.

          Show
          Kathey Marsden added a comment - A couple of things to consider when working on this issue ... 1) This will require more permissons in the testing policy file for derby.jar. It would be nice if they could be added just for sane builds but I don't think that is possible. The two permissions that will be needed to dump the thread stack traces are: permission java.lang.RuntimePermission "getStackTrace"; permission java.lang.RuntimePermission "modifyThreadGroup"; The change should be as such that if these permissions are not present it will just skip the thread dump. 2) Testing might be tricky, since we don't really have JUnit unit testing for internal methods. Maybe it will be easy enough to add if the methods are public. Also the output goes to derby.log which is hard to check. We may just have to settle for manual testing.
          Hide
          Kathey Marsden added a comment -

          To start working with this, you can just use the following program.
          import org.apache.derby.shared.common.sanity.SanityManager;

          public class TestAssert {

          public static void main(String[] args)

          { SanityManager.ASSERT(true == false); }

          }

          It will just print the assertion to stdout. I still don't have a suggestion for automated testing to check the derby.log, but this at least will give you something to get started with. I think the easiest thing will be to add the stack traces to message in AssertFailure before calling super(message).

          Kathey

          Show
          Kathey Marsden added a comment - To start working with this, you can just use the following program. import org.apache.derby.shared.common.sanity.SanityManager; public class TestAssert { public static void main(String[] args) { SanityManager.ASSERT(true == false); } } It will just print the assertion to stdout. I still don't have a suggestion for automated testing to check the derby.log, but this at least will give you something to get started with. I think the easiest thing will be to add the stack traces to message in AssertFailure before calling super(message). Kathey
          Hide
          Bryan Pendleton added a comment -

          > don't have a suggestion for automated testing to check the derby.log

          I'm just blue-sky thinking here, but here are some possible ideas:

          1) Write some common shared utility code that uses java.io.* classes to
          go find the derby.log file on disk and read it.

          2) Inject some sort of "duplicate" or "tee" stream into the output routines
          that write to derby.log so that they can be hooked to emit output both
          to derby.log and to some other stream (say, an in-memory byte-array
          stream) that can then be examined for the content in question.

          3) Change the output routines that write to derby.log so that they have
          some sort of "registered handler" concept so that, immediately prior
          to writing some data to derby.log, they call back to any registered output
          handlers to give the handler a chance to examine the data that's about
          to be written.

          One way to look at these options is whether the test hooks should examine
          the derby.log output after-the-fact, as-the-data-is-being-written, or
          before-the-fact.

          Another way is whether the right abstraction layer is to consider the
          file as a whole as a stream of data, or each individual "record" as it
          is being written to the file.

          And yet another way to look at the options is whether it's easiest to write
          the test code as callback handlers, or as stream-content verifiers. How
          will it be easiest to recognize the logging content that you're looking for?

          Solutions (2) and (3) assume that the test code is in the same JVM as
          the Derby code being tested, so they wouldn't work so well for network
          server tests, although for this particular case that might be just fine.

          Just thought I'd try to throw some ideas out there to encourage the discussion.

          Show
          Bryan Pendleton added a comment - > don't have a suggestion for automated testing to check the derby.log I'm just blue-sky thinking here, but here are some possible ideas: 1) Write some common shared utility code that uses java.io.* classes to go find the derby.log file on disk and read it. 2) Inject some sort of "duplicate" or "tee" stream into the output routines that write to derby.log so that they can be hooked to emit output both to derby.log and to some other stream (say, an in-memory byte-array stream) that can then be examined for the content in question. 3) Change the output routines that write to derby.log so that they have some sort of "registered handler" concept so that, immediately prior to writing some data to derby.log, they call back to any registered output handlers to give the handler a chance to examine the data that's about to be written. One way to look at these options is whether the test hooks should examine the derby.log output after-the-fact, as-the-data-is-being-written, or before-the-fact. Another way is whether the right abstraction layer is to consider the file as a whole as a stream of data, or each individual "record" as it is being written to the file. And yet another way to look at the options is whether it's easiest to write the test code as callback handlers, or as stream-content verifiers. How will it be easiest to recognize the logging content that you're looking for? Solutions (2) and (3) assume that the test code is in the same JVM as the Derby code being tested, so they wouldn't work so well for network server tests, although for this particular case that might be just fine. Just thought I'd try to throw some ideas out there to encourage the discussion.
          Hide
          Erlend Birkenes added a comment -

          Is something like this what you had in mind? (DERBY-3618_1.diff)
          I couldn't figure out how to set up the Monitor in my testprogram, but it should print to the log if that is done. If not it just prints it to System.out instead. How should I set up Monitor to test it in a simple program?

          Show
          Erlend Birkenes added a comment - Is something like this what you had in mind? ( DERBY-3618 _1.diff) I couldn't figure out how to set up the Monitor in my testprogram, but it should print to the log if that is done. If not it just prints it to System.out instead. How should I set up Monitor to test it in a simple program?
          Erlend Birkenes made changes -
          Attachment DERBY-3618_1.diff [ 12382668 ]
          Hide
          Kathey Marsden added a comment -

          Thanks Erlend for the patch.
          – need a license header for ThreadDump

          – AssertFailure
          We can't use org.apache.derbyTesting.functionTests.harness.JavaVersionHolder in the engine code. You can use org.apache.derby.iapi.services.info.JVMInfo instead.

          I think that we can keep the System.out backup if there is no stream, but should check to see if Monitor.getStream() is null instead of catching the NullPointerException.

          I wonder if we could try testing this from within a stored procedure, from there there should be a log stream present.

          Show
          Kathey Marsden added a comment - Thanks Erlend for the patch. – need a license header for ThreadDump – AssertFailure We can't use org.apache.derbyTesting.functionTests.harness.JavaVersionHolder in the engine code. You can use org.apache.derby.iapi.services.info.JVMInfo instead. I think that we can keep the System.out backup if there is no stream, but should check to see if Monitor.getStream() is null instead of catching the NullPointerException. I wonder if we could try testing this from within a stored procedure, from there there should be a log stream present.
          Hide
          Bryan Pendleton added a comment -

          As part of moving ThreadDump from the testing sub-tree to the shared sub-tree,
          we should delete the ThreadDump.java from the testing tree and change all
          uses of it to use the new ThreadDump code in the shared sub-tree.

          Show
          Bryan Pendleton added a comment - As part of moving ThreadDump from the testing sub-tree to the shared sub-tree, we should delete the ThreadDump.java from the testing tree and change all uses of it to use the new ThreadDump code in the shared sub-tree.
          Hide
          Kathey Marsden added a comment -

          I wonder what the status is on sharing classes between jars. There were conversations long ago about the problems this could cause if there were mixed version jars in the classpath, which I don't think were resolved. Is it ok for SanityManager classes because they are only for sane builds. I also thought that in general the testing code was not supposed to use any product code. Even though it's duplicate code. I think I would prefer the testing code be kept separate.

          Show
          Kathey Marsden added a comment - I wonder what the status is on sharing classes between jars. There were conversations long ago about the problems this could cause if there were mixed version jars in the classpath, which I don't think were resolved. Is it ok for SanityManager classes because they are only for sane builds. I also thought that in general the testing code was not supposed to use any product code. Even though it's duplicate code. I think I would prefer the testing code be kept separate.
          Hide
          Bryan Pendleton added a comment -

          Those are all good points, Kathey. I think it's fine to keep the
          separate ThreadDump.java implementations. It isn't a lot of duplicated code.

          Show
          Bryan Pendleton added a comment - Those are all good points, Kathey. I think it's fine to keep the separate ThreadDump.java implementations. It isn't a lot of duplicated code.
          Hide
          Erlend Birkenes added a comment -
          • Added license header
          • changed org.apache.derbyTesting.functionTests.harness.JavaVersionHolder to org.apache.derby.iapi.services.info.JVMInfo
          • changed to check to see if Monitor.getStream() is null instead of catching the NullPointerException.

          Still haven't done anything about testing this. I can't see that there is anything that we really need to test here. The ThreadDump class is very basic and uses only standard java methods and shouldn't need testing.

          In AssertFeilure we just take the dump-string and pass it on to Monitor.getStream().getPrintWriter(). Isn't it the Monitors job to set up the output stream correctly? If the PrintWriter points to derby.log it'll work and if it points to System.err or somewhere else then it's probably because derby.log doesn't exist or there is another good reason for it. In any case there will always be a PrintWriter to write to. But testing that writing to the log file works correctly should be tested elsewhere. We should be able to trust that Monitor.getStream().getPrintWriter() can deal with any string correctly.

          Is there anything else we need to test here?

          Show
          Erlend Birkenes added a comment - Added license header changed org.apache.derbyTesting.functionTests.harness.JavaVersionHolder to org.apache.derby.iapi.services.info.JVMInfo changed to check to see if Monitor.getStream() is null instead of catching the NullPointerException. Still haven't done anything about testing this. I can't see that there is anything that we really need to test here. The ThreadDump class is very basic and uses only standard java methods and shouldn't need testing. In AssertFeilure we just take the dump-string and pass it on to Monitor.getStream().getPrintWriter(). Isn't it the Monitors job to set up the output stream correctly? If the PrintWriter points to derby.log it'll work and if it points to System.err or somewhere else then it's probably because derby.log doesn't exist or there is another good reason for it. In any case there will always be a PrintWriter to write to. But testing that writing to the log file works correctly should be tested elsewhere. We should be able to trust that Monitor.getStream().getPrintWriter() can deal with any string correctly. Is there anything else we need to test here?
          Erlend Birkenes made changes -
          Attachment DERBY-3618_2.diff [ 12384701 ]
          Hide
          Bryan Pendleton added a comment -

          Can we write a test which intentionally performs an AssertFailure, in order to force the
          new code to be executed? Perhaps all the test needs to do is to instantiate an
          AssertFailure object, since the thread dumping occurs in the AssertFailure constructor?

          Show
          Bryan Pendleton added a comment - Can we write a test which intentionally performs an AssertFailure, in order to force the new code to be executed? Perhaps all the test needs to do is to instantiate an AssertFailure object, since the thread dumping occurs in the AssertFailure constructor?
          Hide
          Erlend Birkenes added a comment -

          Ok, It took me a while, but here is version 3.

          I added a jUnit test in org.apache.derbyTesting.unitTests.junit.AssertFailureTest that tests that the output produced by AssertFailure is correct in various situations.

          i also changed the output a bit since last time so it looks better and is more useful. It now always prints the AssertFailure stack trace first and then the thread dump if we can get it, if not it prints the reason why we couldn't. This still gets printed to derby.log if it is available or to System.out if not or if verbose is on.

          I struggled a bit with the permissions for the test, and I ended up with having to give the necessary permissions to all (see org.apache.derbyTesting.unitTests.junit.AssertFailureTest.policy) as this was the only way I could get it to work in all situations, but I'm sure there is a better way to do it. Let me know if you know how.

          Also added the jUnit test to org.apache.derbyTesting.unitTests.junit._Suite.

          Please comment.

          Show
          Erlend Birkenes added a comment - Ok, It took me a while, but here is version 3. I added a jUnit test in org.apache.derbyTesting.unitTests.junit.AssertFailureTest that tests that the output produced by AssertFailure is correct in various situations. i also changed the output a bit since last time so it looks better and is more useful. It now always prints the AssertFailure stack trace first and then the thread dump if we can get it, if not it prints the reason why we couldn't. This still gets printed to derby.log if it is available or to System.out if not or if verbose is on. I struggled a bit with the permissions for the test, and I ended up with having to give the necessary permissions to all (see org.apache.derbyTesting.unitTests.junit.AssertFailureTest.policy) as this was the only way I could get it to work in all situations, but I'm sure there is a better way to do it. Let me know if you know how. Also added the jUnit test to org.apache.derbyTesting.unitTests.junit._Suite. Please comment.
          Erlend Birkenes made changes -
          Attachment DERBY-3618_3.diff [ 12386011 ]
          Hide
          Erlend Birkenes added a comment -

          Sorry, there is something wrong with the latest patch. It won't build properly.
          I'll post a new patch when it's fixed.

          Show
          Erlend Birkenes added a comment - Sorry, there is something wrong with the latest patch. It won't build properly. I'll post a new patch when it's fixed.
          Hide
          Erlend Birkenes added a comment -

          Version 4.

          Fixed a problem in the last patch where a reference to ThreadDump caused the build to fail. This was to get it added to the jars, but this is now properly done by adding it to dnc.properties and extraDBMSclasses.properties.

          But there is still a problem. I'm getting:

          [javac] /home/erlend/workspace/Derby/java/shared/org/apache/derby/shared/common/sanity/AssertFailure.java:32: package org.apache.derby.iapi.services.info does not exist
          [javac] import org.apache.derby.iapi.services.info.JVMInfo;
          [javac] ^
          [javac] /home/erlend/workspace/Derby/java/shared/org/apache/derby/shared/common/sanity/AssertFailure.java:33: package org.apache.derby.iapi.services.monitor does not exist
          [javac] import org.apache.derby.iapi.services.monitor.Monitor;

          This packages are compiled after org/apache/derby/shared/ as part if the engine target, which is depending on the shared target.

          How can I fix this?

          -Erlend

          Show
          Erlend Birkenes added a comment - Version 4. Fixed a problem in the last patch where a reference to ThreadDump caused the build to fail. This was to get it added to the jars, but this is now properly done by adding it to dnc.properties and extraDBMSclasses.properties. But there is still a problem. I'm getting: [javac] /home/erlend/workspace/Derby/java/shared/org/apache/derby/shared/common/sanity/AssertFailure.java:32: package org.apache.derby.iapi.services.info does not exist [javac] import org.apache.derby.iapi.services.info.JVMInfo; [javac] ^ [javac] /home/erlend/workspace/Derby/java/shared/org/apache/derby/shared/common/sanity/AssertFailure.java:33: package org.apache.derby.iapi.services.monitor does not exist [javac] import org.apache.derby.iapi.services.monitor.Monitor; This packages are compiled after org/apache/derby/shared/ as part if the engine target, which is depending on the shared target. How can I fix this? -Erlend
          Erlend Birkenes made changes -
          Attachment DERBY-3618_4.diff [ 12386071 ]
          Hide
          Kathey Marsden added a comment -

          So it looks like you have stumbled upon an old and debated topic of shared code. There was a long conversation about this a long time ago, but basically there are some serious problems with sharing code between engine and client if mixed jar versions are used, because you don't know what version will be used.
          There may also be some issues with sealing violations with shared code that I don't fully understand (see DERBY-1125)

          I thought that the conclusion was that there would be a shared directory created but that that code would actually only go into client until a real shared code solution was found. Also it was determined that it was ok for constants like Attribute.java and SQLState.java to go in because the constants get compiled out.

          Despite that, it looks like the SanityManager code made its way into the shared directory and is actually shared between engine and client.
          I am guessing it was decided that it was ok to share the SanityManager code because it doesn't affect production (insane) builds, but that's just a guess.

          Now for your changes, I don't think Monitor and all its dependencies should be shared code and go into derbyclient.jar. I think the best solution would be to build the trace info into the assertion message and eliminate use of Monitor in the new code. It doesn't make sense when used in the context of client anyway. As for JVMInfo I tend to think as well, that the cleanest thing is not to use it. You can attempt a Class.forName() on a class only available in JDK 1.5 or higher as a quick check of the vm level.

          Show
          Kathey Marsden added a comment - So it looks like you have stumbled upon an old and debated topic of shared code. There was a long conversation about this a long time ago, but basically there are some serious problems with sharing code between engine and client if mixed jar versions are used, because you don't know what version will be used. There may also be some issues with sealing violations with shared code that I don't fully understand (see DERBY-1125 ) I thought that the conclusion was that there would be a shared directory created but that that code would actually only go into client until a real shared code solution was found. Also it was determined that it was ok for constants like Attribute.java and SQLState.java to go in because the constants get compiled out. Despite that, it looks like the SanityManager code made its way into the shared directory and is actually shared between engine and client. I am guessing it was decided that it was ok to share the SanityManager code because it doesn't affect production (insane) builds, but that's just a guess. Now for your changes, I don't think Monitor and all its dependencies should be shared code and go into derbyclient.jar. I think the best solution would be to build the trace info into the assertion message and eliminate use of Monitor in the new code. It doesn't make sense when used in the context of client anyway. As for JVMInfo I tend to think as well, that the cleanest thing is not to use it. You can attempt a Class.forName() on a class only available in JDK 1.5 or higher as a quick check of the vm level.
          Hide
          Erlend Birkenes added a comment -

          Version 5.

          Ok, I changed the whole thing.
          The thread dump is now just added to the message in the AssertFailure so we don't need the Monitor anymore.
          And instead of using JVMInfo I just check that the method Thread.getAllStackTraces is available.

          Please review.

          -Erlend

          Show
          Erlend Birkenes added a comment - Version 5. Ok, I changed the whole thing. The thread dump is now just added to the message in the AssertFailure so we don't need the Monitor anymore. And instead of using JVMInfo I just check that the method Thread.getAllStackTraces is available. Please review. -Erlend
          Erlend Birkenes made changes -
          Attachment DERBY-3618_5.diff [ 12386421 ]
          Hide
          Kathey Marsden added a comment -

          I haven't reviewed the code yet, but I gave the patch on Zos where I am debugging an ASSERT failure in refActions1 (probable jvm issue). I noticed that my assert looked like this. I seemed to lose the actual ASSERT message in this case which should have been:"Container closed while IO operations are in progress."

          Exception trace:
          org.apache.derby.shared.common.sanity.AssertFailure: (Skipping thread dump because of insufficient permissions:
          java.security.AccessControlException: Access denied (java.lang.RuntimePermission getStackTrace))

          at org.apache.derby.shared.common.sanity.SanityManager.ASSERT(SanityManager.java:120)
          at org.apache.derby.impl.store.raw.data.RAFContainer4.closeContainer(RAFContainer4.java:169)
          at org.apache.derby.impl.store.raw.data.FileContainer.clearIdentity(FileContainer.java:473)
          at org.apache.derby.impl.services.cache.ConcurrentCache.evictEntry(ConcurrentCache.java:188)
          at org.apache.derby.impl.services.cache.ClockPolicy.shrinkMe(ClockPolicy.java:658)
          at org.apache.derby.impl.services.cache.ClockPolicy.doShrink(ClockPolicy.java:568)
          at org.apache.derby.impl.services.cache.ClockPolicy.insertEntry(ClockPolicy.java:168)
          at org.apache.derby.impl.services.cache.ConcurrentCache.insertIntoFreeSlot(ConcurrentCache.java:208)
          at org.apache.derby.impl.services.cache.ConcurrentCache.find(ConcurrentCache.java:284)
          at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.openContainer(BaseDataFileFactory.java:628)
          at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.openDroppedContainer(BaseDataFileFactory.java:578)
          at org.apache.derby.impl.store.raw.xact.Xact.openDroppedContainer(Xact.java:1305)
          at org.apache.derby.impl.store.raw.data.ContainerBasicOperation.findContainer(ContainerBasicOperation.java:145)
          at org.apache.derby.impl.store.raw.data.ContainerBasicOperation.needsRedo(ContainerBasicOperation.java:213)
          at org.apache.derby.impl.store.raw.log.FileLogger.redo(FileLogger.java:1395)
          at org.apache.derby.impl.store.raw.log.LogToFile.recover(LogToFile.java:920)
          at org.apache.derby.impl.store.raw.RawStore.boot(RawStore.java:334)
          at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:2000)
          at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
          at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:554)
          at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:427)
          at org.apache.derby.impl.store.access.RAMAccessManager.boot(RAMAccessManager.java:1019)
          at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:2000)
          at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
          at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:554)
          at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:427)
          at org.apache.derby.impl.db.BasicDatabase.bootStore(BasicDatabase.java:780)
          at org.apache.derby.impl.db.BasicDatabase.boot(BasicDatabase.java:196)
          at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:2000)
          at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291)
          at org.apache.derby.impl.services.monitor.BaseMonitor.bootService(BaseMonitor.java:1837)
          at org.apache.derby.impl.services.monitor.BaseMonitor.startProviderService(BaseMonitor.java:1703)
          at org.apache.derby.impl.services.monitor.BaseMonitor.findProviderAndStartService(BaseMonitor.java:1583)
          at org.apache.derby.impl.services.monitor.BaseMonitor.startPersistentService(BaseMonitor.java:1002)
          at org.apache.derby.iapi.services.monitor.Monitor.startPersistentService(Monitor.java:550)
          at org.apache.derby.impl.jdbc.EmbedConnection.bootDatabase(EmbedConnection.java:2572)
          at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.java:365)
          at org.apache.derby.jdbc.Driver40.getNewEmbedConnection(Driver40.java:68)

          I think it is good that it prints that it could not print the stack traces, but it should preserve the original message as well. I'll try changing derby_tests.policy to include the permssions (which I think we should do) and try again.

          Show
          Kathey Marsden added a comment - I haven't reviewed the code yet, but I gave the patch on Zos where I am debugging an ASSERT failure in refActions1 (probable jvm issue). I noticed that my assert looked like this. I seemed to lose the actual ASSERT message in this case which should have been:"Container closed while IO operations are in progress." Exception trace: org.apache.derby.shared.common.sanity.AssertFailure: (Skipping thread dump because of insufficient permissions: java.security.AccessControlException: Access denied (java.lang.RuntimePermission getStackTrace)) at org.apache.derby.shared.common.sanity.SanityManager.ASSERT(SanityManager.java:120) at org.apache.derby.impl.store.raw.data.RAFContainer4.closeContainer(RAFContainer4.java:169) at org.apache.derby.impl.store.raw.data.FileContainer.clearIdentity(FileContainer.java:473) at org.apache.derby.impl.services.cache.ConcurrentCache.evictEntry(ConcurrentCache.java:188) at org.apache.derby.impl.services.cache.ClockPolicy.shrinkMe(ClockPolicy.java:658) at org.apache.derby.impl.services.cache.ClockPolicy.doShrink(ClockPolicy.java:568) at org.apache.derby.impl.services.cache.ClockPolicy.insertEntry(ClockPolicy.java:168) at org.apache.derby.impl.services.cache.ConcurrentCache.insertIntoFreeSlot(ConcurrentCache.java:208) at org.apache.derby.impl.services.cache.ConcurrentCache.find(ConcurrentCache.java:284) at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.openContainer(BaseDataFileFactory.java:628) at org.apache.derby.impl.store.raw.data.BaseDataFileFactory.openDroppedContainer(BaseDataFileFactory.java:578) at org.apache.derby.impl.store.raw.xact.Xact.openDroppedContainer(Xact.java:1305) at org.apache.derby.impl.store.raw.data.ContainerBasicOperation.findContainer(ContainerBasicOperation.java:145) at org.apache.derby.impl.store.raw.data.ContainerBasicOperation.needsRedo(ContainerBasicOperation.java:213) at org.apache.derby.impl.store.raw.log.FileLogger.redo(FileLogger.java:1395) at org.apache.derby.impl.store.raw.log.LogToFile.recover(LogToFile.java:920) at org.apache.derby.impl.store.raw.RawStore.boot(RawStore.java:334) at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:2000) at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291) at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:554) at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:427) at org.apache.derby.impl.store.access.RAMAccessManager.boot(RAMAccessManager.java:1019) at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:2000) at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291) at org.apache.derby.impl.services.monitor.BaseMonitor.startModule(BaseMonitor.java:554) at org.apache.derby.iapi.services.monitor.Monitor.bootServiceModule(Monitor.java:427) at org.apache.derby.impl.db.BasicDatabase.bootStore(BasicDatabase.java:780) at org.apache.derby.impl.db.BasicDatabase.boot(BasicDatabase.java:196) at org.apache.derby.impl.services.monitor.BaseMonitor.boot(BaseMonitor.java:2000) at org.apache.derby.impl.services.monitor.TopService.bootModule(TopService.java:291) at org.apache.derby.impl.services.monitor.BaseMonitor.bootService(BaseMonitor.java:1837) at org.apache.derby.impl.services.monitor.BaseMonitor.startProviderService(BaseMonitor.java:1703) at org.apache.derby.impl.services.monitor.BaseMonitor.findProviderAndStartService(BaseMonitor.java:1583) at org.apache.derby.impl.services.monitor.BaseMonitor.startPersistentService(BaseMonitor.java:1002) at org.apache.derby.iapi.services.monitor.Monitor.startPersistentService(Monitor.java:550) at org.apache.derby.impl.jdbc.EmbedConnection.bootDatabase(EmbedConnection.java:2572) at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.java:365) at org.apache.derby.jdbc.Driver40.getNewEmbedConnection(Driver40.java:68) I think it is good that it prints that it could not print the stack traces, but it should preserve the original message as well. I'll try changing derby_tests.policy to include the permssions (which I think we should do) and try again.
          Hide
          Kathey Marsden added a comment -

          Thanks Erlend for the new patch. I think we are getting very close. Per earlier comment need to ...

          • Fix it so ASSERT message prints even if you don't have permission to dump threads.
          • add permissions to derby_tests.policy to derby.jar and derbyclient.jar to have permissions to dump threads.

          I tried adding the permissions and with the permissions it worked fine with my assertion and printed the message properly. It is too bad that our exception stack trace has to print after the thread dump, but I guess there is no way around it now that the thread dump is part of the message?

          On the tests, I don't see actually where setIO permissions are needed. Is this perhaps residual from when the thread dump printed to System.out?

          You should be able to give the getStackTrace and modifyThreadGroup permissions to derby.jar and derbyclient.jar and avoid having to grant permissions to all. If you have to grant permissions to all there is some sort of problem with a privileged block missing somwhere or something like that. Take a look at derby_tests.policy for default derby.jar, derbyclient.jar permissions.

          Thanks again Erlend for all the great work. This is indeed tricky stuff.

          Show
          Kathey Marsden added a comment - Thanks Erlend for the new patch. I think we are getting very close. Per earlier comment need to ... Fix it so ASSERT message prints even if you don't have permission to dump threads. add permissions to derby_tests.policy to derby.jar and derbyclient.jar to have permissions to dump threads. I tried adding the permissions and with the permissions it worked fine with my assertion and printed the message properly. It is too bad that our exception stack trace has to print after the thread dump, but I guess there is no way around it now that the thread dump is part of the message? On the tests, I don't see actually where setIO permissions are needed. Is this perhaps residual from when the thread dump printed to System.out? You should be able to give the getStackTrace and modifyThreadGroup permissions to derby.jar and derbyclient.jar and avoid having to grant permissions to all. If you have to grant permissions to all there is some sort of problem with a privileged block missing somwhere or something like that. Take a look at derby_tests.policy for default derby.jar, derbyclient.jar permissions. Thanks again Erlend for all the great work. This is indeed tricky stuff.
          Hide
          Kathey Marsden added a comment -

          Oh one more thing on the tests. You have
          if (JVMInfo.JDK_ID >= 6) {
          ... test

          should it b
          if (JVMInfo.JDK_ID >= 5) {
          ?

          Show
          Kathey Marsden added a comment - Oh one more thing on the tests. You have if (JVMInfo.JDK_ID >= 6) { ... test should it b if (JVMInfo.JDK_ID >= 5) { ?
          Hide
          Erlend Birkenes added a comment -

          No, this is correct. 5 is 1.4.2, 6 is 1.5. Misleading, i know..

          Show
          Erlend Birkenes added a comment - No, this is correct. 5 is 1.4.2, 6 is 1.5. Misleading, i know..
          Hide
          Erlend Birkenes added a comment -

          Version 6.

          • Fixed it so ASSERT message always prints, as it indeed was meant to, but I had missed it.
          • Added permissions to derby_tests.policy to derby.jar and derbyclient.jar to have permissions to dump threads.
          • Removed setIO prom the policies. That was leftovers from the first test.
          • Given permissions to derby.jar and derbyclient.jar instead of all in the test policy. That worked fine. Thanks for the tip

          -Unfortunately the exception stack trace has to come after the thread dump when we do it this way, as the message can only be set in the constructor, the super constructor has to be called first, and non-static methods can't be called in it so we can't get to the stack trace until after the superconstructor. I tried everything to get it into the message, but I don't think it can be done.

          Show
          Erlend Birkenes added a comment - Version 6. Fixed it so ASSERT message always prints, as it indeed was meant to, but I had missed it. Added permissions to derby_tests.policy to derby.jar and derbyclient.jar to have permissions to dump threads. Removed setIO prom the policies. That was leftovers from the first test. Given permissions to derby.jar and derbyclient.jar instead of all in the test policy. That worked fine. Thanks for the tip -Unfortunately the exception stack trace has to come after the thread dump when we do it this way, as the message can only be set in the constructor, the super constructor has to be called first, and non-static methods can't be called in it so we can't get to the stack trace until after the superconstructor. I tried everything to get it into the message, but I don't think it can be done.
          Erlend Birkenes made changes -
          Attachment DERBY-3618_6.diff [ 12386455 ]
          Hide
          Erlend Birkenes added a comment -

          Version 7.

          I must have been really sleepy when I said something couldn't be done, because obviously anything can be done

          A simple override of the printStackTrace methods did what Kathey wanted. It now prints the full stack trace of the error and then the thread dump to System.err.
          The thread dump is no longer part of the the message, wich was kind of a ugly hack anyway, but stored in its own field and accessible by getThreadDump() if someone wants it.

          -E

          Show
          Erlend Birkenes added a comment - Version 7. I must have been really sleepy when I said something couldn't be done, because obviously anything can be done A simple override of the printStackTrace methods did what Kathey wanted. It now prints the full stack trace of the error and then the thread dump to System.err. The thread dump is no longer part of the the message, wich was kind of a ugly hack anyway, but stored in its own field and accessible by getThreadDump() if someone wants it. -E
          Erlend Birkenes made changes -
          Attachment DERBY-3618_7.diff [ 12386488 ]
          Hide
          Kathey Marsden added a comment -

          Thanks Erlend,

          Looks and runs great. I'll run tests and check it in tomorrow. It would be great however to get another set of eyes on this if someone else has time to look.

          Kathey

          Show
          Kathey Marsden added a comment - Thanks Erlend, Looks and runs great. I'll run tests and check it in tomorrow. It would be great however to get another set of eyes on this if someone else has time to look. Kathey
          Hide
          Kathey Marsden added a comment -

          Committed revision 678858. I made a few minor changes to the test so that it would work on various jvm's. Thanks Erlend for all of your hard work on this patch.

          Show
          Kathey Marsden added a comment - Committed revision 678858. I made a few minor changes to the test so that it would work on various jvm's. Thanks Erlend for all of your hard work on this patch.
          Erlend Birkenes made changes -
          Fix Version/s 10.5.0.0 [ 12313010 ]
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Knut Anders Hatlen added a comment -

          A couple of comments to the version 7 patch:

          • The description of the issue says that the thread dump should be printed to derby.log, but this patch seems to put the thread dump in the output from printStackTrace(), which goes much broader. Doesn't this make the stack traces harder to read, and debugging problems where we don't care about the thread dumps harder? Perhaps it's better to intercept AssertFailure (or perhaps any error?) in BaseTestCase.runBare() and write the thread dump to a log file?
          • The added code in AssertFailure.java is inconsistently indented (some lines use tabs, others use spaces, and some use a mix of tabs and spaces)
          • Is the three level nesting of try/catch blocks in AssertFailure.dumpThreads() needed? Could they be merged into one?
          • The outer catch block in dumpThreads() has an instanceof test and a cast. Could it instead have a separate catch clause for InvocationTargetException?
          • In dumpThread(), this code could be simplified to Thread.class.getMethod(...):
            + Class c = Class.forName("java.lang.Thread");
            + c.getMethod("getAllStackTraces", new Class[] {});
          • Perhaps a StringWriter is more appropriate than a ByteArrayOutputStream to generate the string in dumpThread()?
          • ThreadDump.java has the wrong package name in its header.
          • AssertFailureTest: Use constant JVMInfo.J2SE_15 for readability?
          Show
          Knut Anders Hatlen added a comment - A couple of comments to the version 7 patch: The description of the issue says that the thread dump should be printed to derby.log, but this patch seems to put the thread dump in the output from printStackTrace(), which goes much broader. Doesn't this make the stack traces harder to read, and debugging problems where we don't care about the thread dumps harder? Perhaps it's better to intercept AssertFailure (or perhaps any error?) in BaseTestCase.runBare() and write the thread dump to a log file? The added code in AssertFailure.java is inconsistently indented (some lines use tabs, others use spaces, and some use a mix of tabs and spaces) Is the three level nesting of try/catch blocks in AssertFailure.dumpThreads() needed? Could they be merged into one? The outer catch block in dumpThreads() has an instanceof test and a cast. Could it instead have a separate catch clause for InvocationTargetException? In dumpThread(), this code could be simplified to Thread.class.getMethod(...): + Class c = Class.forName("java.lang.Thread"); + c.getMethod("getAllStackTraces", new Class[] {}); Perhaps a StringWriter is more appropriate than a ByteArrayOutputStream to generate the string in dumpThread()? ThreadDump.java has the wrong package name in its header. AssertFailureTest: Use constant JVMInfo.J2SE_15 for readability?
          Hide
          Knut Anders Hatlen added a comment -

          Oops. Too late with my comment. Sorry about that, but perhaps some of the comments could be addressed in followup patches?

          Show
          Knut Anders Hatlen added a comment - Oops. Too late with my comment. Sorry about that, but perhaps some of the comments could be addressed in followup patches?
          Hide
          Kathey Marsden added a comment -

          Sorry Knut I committed before you got your comments in. Please let me know if you think I should back out the change. I have one comment on your comments. You said:
          > - The description of the issue says that the thread dump should be printed to >derby.log, but this patch seems to put the thread dump in the output from >printStackTrace(), which goes much broader. Doesn't this make the stack traces harder >to read, and debugging problems where we don't care about the thread dumps harder? >Perhaps it's better to intercept AssertFailure (or perhaps any error?) in >BaseTestCase.runBare() and write the thread dump to a log file?

          We sometimes ask users to try to reproduce failures with sane builds in their environments and could use the thread dumps in those instances. Having the thread dump code in the junit test harness won't help in these cases, nor will it help if the assertion happens in derbyall. I think the change won't make assertions harder to read, since the stack trace will be right at the top and the thread dump can be ignored if so desired.

          Show
          Kathey Marsden added a comment - Sorry Knut I committed before you got your comments in. Please let me know if you think I should back out the change. I have one comment on your comments. You said: > - The description of the issue says that the thread dump should be printed to >derby.log, but this patch seems to put the thread dump in the output from >printStackTrace(), which goes much broader. Doesn't this make the stack traces harder >to read, and debugging problems where we don't care about the thread dumps harder? >Perhaps it's better to intercept AssertFailure (or perhaps any error?) in >BaseTestCase.runBare() and write the thread dump to a log file? We sometimes ask users to try to reproduce failures with sane builds in their environments and could use the thread dumps in those instances. Having the thread dump code in the junit test harness won't help in these cases, nor will it help if the assertion happens in derbyall. I think the change won't make assertions harder to read, since the stack trace will be right at the top and the thread dump can be ignored if so desired.
          Hide
          Knut Anders Hatlen added a comment -

          Ah, OK, I think I see now. Since the AssertFailures normally will be wrapped in SQLExceptions before they are presented to the users, the thread dump won't be printed to the console. However, when the exception is written to derby.log, it is unwrapped, so the thread dump will be picked up by calling printStackTrace() on the unwrapped AssertFailure object. Is this about correct?

          Show
          Knut Anders Hatlen added a comment - Ah, OK, I think I see now. Since the AssertFailures normally will be wrapped in SQLExceptions before they are presented to the users, the thread dump won't be printed to the console. However, when the exception is written to derby.log, it is unwrapped, so the thread dump will be picked up by calling printStackTrace() on the unwrapped AssertFailure object. Is this about correct?
          Hide
          Knut Anders Hatlen added a comment -

          Sorry for asking all these questions, but what makes an AssertFailure special compared to, say, a NullPointerException? Instead of limiting this functionality to AssertFailures, should we have a property (derby.stream.error.dumpStackTrace or something) that we could use to turn on thread dumps for all exceptions in derby.log, also in production code?

          Show
          Knut Anders Hatlen added a comment - Sorry for asking all these questions, but what makes an AssertFailure special compared to, say, a NullPointerException? Instead of limiting this functionality to AssertFailures, should we have a property (derby.stream.error.dumpStackTrace or something) that we could use to turn on thread dumps for all exceptions in derby.log, also in production code?
          Hide
          Kathey Marsden added a comment -

          Knut asked:
          >Since the AssertFailures normally will be wrapped in SQLExceptions before they are presented to >the users, the thread dump won't be printed to the console. However, when the exception is >written to derby.log, it is unwrapped, so the thread dump will be picked up by calling >printStackTrace() on the unwrapped AssertFailure object. Is this about correct?

          I verified that this is the case. When I run a test with an assertion failure I see only the stack trace printed in the test failures and see the thread dump in the derby.log.

          >but what makes an AssertFailure special compared to, say, a NullPointerException? Instead of >limiting this functionality to AssertFailures, should we have a property >(derby.stream.error.dumpStackTrace or something) that we could use to turn on thread dumps for >all exceptions in derby.log, also in production code?

          That sounds like a great improvement. This issue was just focussed on assertions. That idea would cover DERBY-666 as well. One thing to consider is that if it goes into production code, the ThreadDump class should no longer be shared.

          Show
          Kathey Marsden added a comment - Knut asked: >Since the AssertFailures normally will be wrapped in SQLExceptions before they are presented to >the users, the thread dump won't be printed to the console. However, when the exception is >written to derby.log, it is unwrapped, so the thread dump will be picked up by calling >printStackTrace() on the unwrapped AssertFailure object. Is this about correct? I verified that this is the case. When I run a test with an assertion failure I see only the stack trace printed in the test failures and see the thread dump in the derby.log. >but what makes an AssertFailure special compared to, say, a NullPointerException? Instead of >limiting this functionality to AssertFailures, should we have a property >(derby.stream.error.dumpStackTrace or something) that we could use to turn on thread dumps for >all exceptions in derby.log, also in production code? That sounds like a great improvement. This issue was just focussed on assertions. That idea would cover DERBY-666 as well. One thing to consider is that if it goes into production code, the ThreadDump class should no longer be shared.
          Hide
          Kathey Marsden added a comment -

          I noticed errors in the regresssion tests. I am not sure why we are getting a ClassNotFound error. I didn't see that in my testing.
          http://dbtg.thresher.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/678939-org.apache.derbyTesting.functionTests.suites.All_diff.txt

          1) testAssertFailureThreadDump(org.apache.derbyTesting.unitTests.junit.AssertFailureTest)java.lang.NoClassDefFoundError: org/apache/derby/shared/common/sanity/AssertFailure
          at org.apache.derbyTesting.unitTests.junit.AssertFailureTest.testAssertFailureThreadDump(AssertFailureTest.java:78)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:104)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          Caused by: java.lang.ClassNotFoundException: org.apache.derby.shared.common.sanity.AssertFailure
          at java.net.URLClassLoader$1.run(URLClassLoader.java:200)
          at java.security.AccessController.doPrivileged(Native Method)
          at java.net.URLClassLoader.findClass(URLClassLoader.java:188)
          at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
          at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:276)
          at java.lang.ClassLoader.loadClass(ClassLoader.java:251)
          at java.lang.ClassLoader.loadClassInternal(ClassLoader.java:319)
          ... 27 more
          2) testAssertFailureNoThreadDump(org.apache.derbyTesting.unitTests.junit.AssertFailureTest)java.lang.NoClassDefFoundError: org/apache/derby/shared/common/sanity/AssertFailure
          at org.apache.derbyTesting.unitTests.junit.AssertFailureTest.testAssertFailureNoThreadDump(AssertFailureTest.java:107)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:104)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)

          Show
          Kathey Marsden added a comment - I noticed errors in the regresssion tests. I am not sure why we are getting a ClassNotFound error. I didn't see that in my testing. http://dbtg.thresher.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/678939-org.apache.derbyTesting.functionTests.suites.All_diff.txt 1) testAssertFailureThreadDump(org.apache.derbyTesting.unitTests.junit.AssertFailureTest)java.lang.NoClassDefFoundError: org/apache/derby/shared/common/sanity/AssertFailure at org.apache.derbyTesting.unitTests.junit.AssertFailureTest.testAssertFailureThreadDump(AssertFailureTest.java:78) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:104) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) Caused by: java.lang.ClassNotFoundException: org.apache.derby.shared.common.sanity.AssertFailure at java.net.URLClassLoader$1.run(URLClassLoader.java:200) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:188) at java.lang.ClassLoader.loadClass(ClassLoader.java:306) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:276) at java.lang.ClassLoader.loadClass(ClassLoader.java:251) at java.lang.ClassLoader.loadClassInternal(ClassLoader.java:319) ... 27 more 2) testAssertFailureNoThreadDump(org.apache.derbyTesting.unitTests.junit.AssertFailureTest)java.lang.NoClassDefFoundError: org/apache/derby/shared/common/sanity/AssertFailure at org.apache.derbyTesting.unitTests.junit.AssertFailureTest.testAssertFailureNoThreadDump(AssertFailureTest.java:107) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:104) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25)
          Hide
          Kathey Marsden added a comment -

          Of course its because its an insane build. We will need to disable the test for tests with insane jars.

          Show
          Kathey Marsden added a comment - Of course its because its an insane build. We will need to disable the test for tests with insane jars.
          Hide
          Erlend Birkenes added a comment -

          Version 8: DERBY-3618_8.diff

          Thanks for the comments Knut Anders! Better late than never This patch fixes most of the things you noted:

          • Fixed indentation to use only spaces.
          • Merged nested try/catch blocks in AssertFailure.dumpThreads() into one. We cant simply use a catch clause for InvocationTargetException, because the exception thrown is a PrivilegedActionException, containing an InvocationTargetException, containing an AccessControlException which is the one we really want. I changed the Exception clause instead to have only one if/else. Looks much better now and makes more sense. Also by using getCause() instead of getTargetException the cast was made superfluous.
          • Simplified code in dumpThread() to Thread.class.getMethod(...) as you suggested.
          • Changed ByteArrayOutputStream to StringWriter. Don't know what I was thinking here...
          • Corrected package name in ThreadDump.java header.
          • AssertFailureTest now use constant JVMInfo.J2SE_15 for better readability.
          • AssertFailureTest.suite() now returns an empty suite in insane builds. (If org.apache.derby.shared.common.sanity.AssertFailure doesn't exist.) This fixes the error in the regression tests.

          -Erlend

          Show
          Erlend Birkenes added a comment - Version 8: DERBY-3618 _8.diff Thanks for the comments Knut Anders! Better late than never This patch fixes most of the things you noted: Fixed indentation to use only spaces. Merged nested try/catch blocks in AssertFailure.dumpThreads() into one. We cant simply use a catch clause for InvocationTargetException, because the exception thrown is a PrivilegedActionException, containing an InvocationTargetException, containing an AccessControlException which is the one we really want. I changed the Exception clause instead to have only one if/else. Looks much better now and makes more sense. Also by using getCause() instead of getTargetException the cast was made superfluous. Simplified code in dumpThread() to Thread.class.getMethod(...) as you suggested. Changed ByteArrayOutputStream to StringWriter. Don't know what I was thinking here... Corrected package name in ThreadDump.java header. AssertFailureTest now use constant JVMInfo.J2SE_15 for better readability. AssertFailureTest.suite() now returns an empty suite in insane builds. (If org.apache.derby.shared.common.sanity.AssertFailure doesn't exist.) This fixes the error in the regression tests. -Erlend
          Erlend Birkenes made changes -
          Attachment DERBY-3618_8.diff [ 12386793 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the new patch, Erlend. The changes look good to me.
          Committed revision 679742.

          Show
          Knut Anders Hatlen added a comment - Thanks for the new patch, Erlend. The changes look good to me. Committed revision 679742.
          Hide
          Erlend Birkenes added a comment -

          Great.

          'Then this issue is ready to be closed.
          Maybe a new jira should be created for Knut Anders' idea about thread dumps for all exceptions?

          -Erlend

          Show
          Erlend Birkenes added a comment - Great. 'Then this issue is ready to be closed. Maybe a new jira should be created for Knut Anders' idea about thread dumps for all exceptions? -Erlend
          Hide
          Knut Anders Hatlen added a comment -

          I agree. This issue has been solved and can be closed. Adding a property which controls thread dump for all exceptions is another improvement and should be tracked in a separate JIRA issue. Thanks for all the work on this issue!

          Show
          Knut Anders Hatlen added a comment - I agree. This issue has been solved and can be closed. Adding a property which controls thread dump for all exceptions is another improvement and should be tracked in a separate JIRA issue. Thanks for all the work on this issue!
          Kathey Marsden made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Myrna van Lunteren made changes -
          Affects Version/s 10.5.1.1 [ 12313771 ]
          Affects Version/s 10.5.0.0 [ 12313010 ]
          Fix Version/s 10.5.1.1 [ 12313771 ]
          Fix Version/s 10.5.0.0 [ 12313010 ]
          Gavin made changes -
          Workflow jira [ 12428949 ] Default workflow, editable Closed status [ 12798870 ]

            People

            • Assignee:
              Erlend Birkenes
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development