Derby
  1. Derby
  2. DERBY-4443

Wrap rollback in exception handlers in try-catch

    Details

    • Urgency:
      Low
    • Issue & fix info:
      Newcomer

      Description

      Avoid this pattern everywhere:

      }catch(SQLException se)

      { //issue a rollback on any errors conn.rollback(); throw se; }

      because an error in rollback will shadow the original exception.

      1. DERBY-4443.patch
        3 kB
        Houx Zhang
      2. DERBY-4443-1.patch
        7 kB
        Houx Zhang
      3. DERBY-4443-2.patch
        7 kB
        Houx Zhang
      4. DERBY-4443-3.patch
        8 kB
        Houx Zhang
      5. DERBY-4443-4.patch
        10 kB
        Houx Zhang
      6. DERBY-4443-4.png
        15 kB
        Houx Zhang
      7. DERBY-4443-5.patch
        10 kB
        Houx Zhang
      8. DERBY-4443-6-tmp.patch
        10 kB
        Houx Zhang
      9. derby-4443-7.patch
        10 kB
        Houx Zhang

        Issue Links

          Activity

          Hide
          Aaron Digulla added a comment -

          One place where I've seen this is SystemProcedures. Every method in there uses this anti-pattern.

          Instead of calling rollback directly, a utility function should be defined which either ignores any exceptions in rollback or, preferred, prints/logs the exception.

          Show
          Aaron Digulla added a comment - One place where I've seen this is SystemProcedures. Every method in there uses this anti-pattern. Instead of calling rollback directly, a utility function should be defined which either ignores any exceptions in rollback or, preferred, prints/logs the exception.
          Hide
          Mike Matrigali added a comment -

          removing store, as it does not have access to the connection or to SQLException.

          Show
          Mike Matrigali added a comment - removing store, as it does not have access to the connection or to SQLException.
          Hide
          Kathey Marsden added a comment -

          Triaged for 10.8. A solution would be to chain the SQLExceptions so both would be returned to the user. Good newcomer issue.

          Show
          Kathey Marsden added a comment - Triaged for 10.8. A solution would be to chain the SQLExceptions so both would be returned to the user. Good newcomer issue.
          Hide
          Houx Zhang added a comment - - edited

          I think it's a good point for a new comer. I have provided a patch. In my patch, a method named rollBackWhenSQLExceptionOccurs() is added. I'm not sure there is any other place to use this method , if so, it's better to add this method in an independent util class.

          Please check it and give some advice, thanks!

          Show
          Houx Zhang added a comment - - edited I think it's a good point for a new comer. I have provided a patch. In my patch, a method named rollBackWhenSQLExceptionOccurs() is added. I'm not sure there is any other place to use this method , if so, it's better to add this method in an independent util class. Please check it and give some advice, thanks!
          Hide
          Bryan Pendleton added a comment -

          It would be nice if the patch could include a new test which demonstrates the modified behavior
          that occurs with the patch.

          Show
          Bryan Pendleton added a comment - It would be nice if the patch could include a new test which demonstrates the modified behavior that occurs with the patch.
          Hide
          Houx Zhang added a comment -

          It's a good suggestion, Bryan.

          In fact, I have tried to do some unit testing before submitting the patch. However, I've found neither of the four methods which are affected in this patch is called through out the derby project. Furthermore there is not a test class named with "SystemProceduresTest". So, I'm puzzled where to add the test cases.

          Please give a further advice to help me advance!

          Thanks a lot!

          Show
          Houx Zhang added a comment - It's a good suggestion, Bryan. In fact, I have tried to do some unit testing before submitting the patch. However, I've found neither of the four methods which are affected in this patch is called through out the derby project. Furthermore there is not a test class named with "SystemProceduresTest". So, I'm puzzled where to add the test cases. Please give a further advice to help me advance! Thanks a lot!
          Hide
          Bryan Pendleton added a comment -

          I think that the system procedures are called through a clever sort of reflection mechanism, which is why
          you don't see a direct call mechanism in place. Here's a link to these procedures, showing how to
          invoke them: http://db.apache.org/derby/docs/10.7/ref/crefsqlbuiltinsystemprocedures.html

          Regarding an existing test suite, I think that this test suite should run these procedures:
          org.apache.derbyTesting.functionTests.tests.tools.ImportExportProcedureTest
          The code for that test is in
          java/testing/org/apache/derbyTesting/functionTests/tests/tools/ImportExportProcedureTest.java

          Show
          Bryan Pendleton added a comment - I think that the system procedures are called through a clever sort of reflection mechanism, which is why you don't see a direct call mechanism in place. Here's a link to these procedures, showing how to invoke them: http://db.apache.org/derby/docs/10.7/ref/crefsqlbuiltinsystemprocedures.html Regarding an existing test suite, I think that this test suite should run these procedures: org.apache.derbyTesting.functionTests.tests.tools.ImportExportProcedureTest The code for that test is in java/testing/org/apache/derbyTesting/functionTests/tests/tools/ImportExportProcedureTest.java
          Hide
          Houx Zhang added a comment -

          Thanks very much, Bryan! As I have been busy with my school affairs, I has just seen your advice, and it is valuable for me. I will go on, and provide a new patch some days later.

          Show
          Houx Zhang added a comment - Thanks very much, Bryan! As I have been busy with my school affairs, I has just seen your advice, and it is valuable for me. I will go on, and provide a new patch some days later.
          Hide
          Houx Zhang added a comment -

          Hi, Bryan. I have still another question: How to invoke a failure of "roll back" in testing? I need to invoke failures of "roll back" to validate my checking. Thanks!

          Show
          Houx Zhang added a comment - Hi, Bryan. I have still another question: How to invoke a failure of "roll back" in testing? I need to invoke failures of "roll back" to validate my checking. Thanks!
          Hide
          Bryan Pendleton added a comment -

          I think this may be rather tricky. Perhaps other developers have a good idea. One technique,
          which is artificial, is to deliberate modify the code for rolllback so that it fails. Of course,
          that would not be a permanent change, just a temporary one so that you can exercise your
          other altered code paths.

          I suspect that this modification may be rather a hard one to test, unfortunately, so you
          may have to resort to some unusual techniques.

          Show
          Bryan Pendleton added a comment - I think this may be rather tricky. Perhaps other developers have a good idea. One technique, which is artificial, is to deliberate modify the code for rolllback so that it fails. Of course, that would not be a permanent change, just a temporary one so that you can exercise your other altered code paths. I suspect that this modification may be rather a hard one to test, unfortunately, so you may have to resort to some unusual techniques.
          Hide
          Aaron Digulla added a comment -

          The usual solution is to use a mock connection that throws Exceptions when rollback() is called.

          If you look at the code, then you'll see that the connection is created with a call to getDefaultConn(). This will query the InternalDriver for a connection.

          In your tests, extend InternalDriver and override connect() to return a connection object which throws exceptions for prepareStatement() and rollback():

          @Override
          public PreparedStatement prepareStatement(String sql)

          { throw SQLException( "Error in prepareStatement" ); }

          @Override
          public void rollback()

          { throw SQLException( "Error in rollback" ); }

          Make sure that correct exception is visible outside.

          Note that this messes with internal static variables. To fix that, you may need to create a new driver instance in the tearDown() of your tests.

          Show
          Aaron Digulla added a comment - The usual solution is to use a mock connection that throws Exceptions when rollback() is called. If you look at the code, then you'll see that the connection is created with a call to getDefaultConn(). This will query the InternalDriver for a connection. In your tests, extend InternalDriver and override connect() to return a connection object which throws exceptions for prepareStatement() and rollback(): @Override public PreparedStatement prepareStatement(String sql) { throw SQLException( "Error in prepareStatement" ); } @Override public void rollback() { throw SQLException( "Error in rollback" ); } Make sure that correct exception is visible outside. Note that this messes with internal static variables. To fix that, you may need to create a new driver instance in the tearDown() of your tests.
          Hide
          Houx Zhang added a comment -

          Thanks for your advice, Aaron.

          Mock extending InternalDriver seems a good idea. Overriding prepareStatement(String sql) can throw a SQLException for invoking code for rollback, and overriding rollback() can test the new wrapping!

          However, I'm still puzzled. It's related to a key problem---- How to inject the mock for SystemProcedures when testing?

          Wish for your reply!

          Show
          Houx Zhang added a comment - Thanks for your advice, Aaron. Mock extending InternalDriver seems a good idea. Overriding prepareStatement(String sql) can throw a SQLException for invoking code for rollback, and overriding rollback() can test the new wrapping! However, I'm still puzzled. It's related to a key problem---- How to inject the mock for SystemProcedures when testing? Wish for your reply!
          Hide
          Aaron Digulla added a comment -

          Run the code in a debugger. You will see that the static variable which contains the current InternalDriver is set in the constructor of InternalDriver. So you just need to extend the class, call new on your version and call the system procedure.

          Show
          Aaron Digulla added a comment - Run the code in a debugger. You will see that the static variable which contains the current InternalDriver is set in the constructor of InternalDriver. So you just need to extend the class, call new on your version and call the system procedure.
          Hide
          Houx Zhang added a comment -

          Yes, Aaron, I have seen the static variable set in the constructor of InternalDriver in the prospective of debugging when Driver40 is installed. As I'm not familiar with DERBY, could you tell me how to "call new on your version", please? That is, how to install my mocking subclass of InternalDriver in my testing.

          Thanks very much!

          Show
          Houx Zhang added a comment - Yes, Aaron, I have seen the static variable set in the constructor of InternalDriver in the prospective of debugging when Driver40 is installed. As I'm not familiar with DERBY, could you tell me how to "call new on your version", please? That is, how to install my mocking subclass of InternalDriver in my testing. Thanks very much!
          Hide
          Aaron Digulla added a comment -

          I think this is beyond the scope of the bug report. Please ask your questions on stackoverflow.com.

          Show
          Aaron Digulla added a comment - I think this is beyond the scope of the bug report. Please ask your questions on stackoverflow.com.
          Hide
          Bryan Pendleton added a comment -

          Often I find that the best way to move forward with a problem is to try writing the code that
          you think you want to write (in this case, a class that would sub-class InternalDriver, perhaps
          named MockInternalDriver), try
          placing a call to
          InternalDriver myDriver = new MockInternalDriver();
          somewhere in the setUp() method of ImportExportProcedureTest, try it out, and
          see what happens.

          You can probably put the MockInternalDriver code directly into ImportExportProcedureTest,
          as a nested class, so you don't have to create any new source files to do this.

          Put some System.out.println statements into your mock class, so you can see
          whether it is getting run or not, and where it is being used.

          It probably won't work. But, then you can attach an updated patch, containing the
          actual code that you tried, and describe what it did and what you wanted it to do instead.

          In essence: try writing some code, see how far you get, atttach the code you tried, and
          a description of where exactly you got stuck, and the community will offer some
          suggestions about what to try next.

          Show
          Bryan Pendleton added a comment - Often I find that the best way to move forward with a problem is to try writing the code that you think you want to write (in this case, a class that would sub-class InternalDriver, perhaps named MockInternalDriver), try placing a call to InternalDriver myDriver = new MockInternalDriver(); somewhere in the setUp() method of ImportExportProcedureTest, try it out, and see what happens. You can probably put the MockInternalDriver code directly into ImportExportProcedureTest, as a nested class, so you don't have to create any new source files to do this. Put some System.out.println statements into your mock class, so you can see whether it is getting run or not, and where it is being used. It probably won't work. But, then you can attach an updated patch, containing the actual code that you tried, and describe what it did and what you wanted it to do instead. In essence: try writing some code, see how far you get, atttach the code you tried, and a description of where exactly you got stuck, and the community will offer some suggestions about what to try next.
          Hide
          Houx Zhang added a comment -

          Oh, thanks, Bryan. Your advice is great! I will remember to give a patch before launch a discussion.

          I have written some testing code before, just like I do in the new patch. In the patch, I do "new MockInternalDriver();" in the setup() method of my junit test class, however, this sentence will throw a exception as InternalDriver() will check its contextServiceFactory is null.

          I think the key point is to decide where to install the mock of InternalDriver to give it a normal running circumstance, maybe it's related to some .

          Comments are welcome.

          Show
          Houx Zhang added a comment - Oh, thanks, Bryan. Your advice is great! I will remember to give a patch before launch a discussion. I have written some testing code before, just like I do in the new patch. In the patch, I do "new MockInternalDriver();" in the setup() method of my junit test class, however, this sentence will throw a exception as InternalDriver() will check its contextServiceFactory is null. I think the key point is to decide where to install the mock of InternalDriver to give it a normal running circumstance, maybe it's related to some . Comments are welcome.
          Hide
          Bryan Pendleton added a comment -

          I modified the setUp() method of your patch so that it looks like this:

          protected void setUp()
          throws Exception

          { System.out.println("setUp is called!"); openDefaultConnection(); MockInternalDriver dvr = new MockInternalDriver(); dvr.stop(); dvr.boot(false, null); }

          and now the test gets considerably further. It gets to the point where
          it tries to make a new connection, and then fails.

          I think that your connect() method in your MockInternalDriver
          must do more than just return an empty MockConnection().

          I think that you have to call super.connect(url, info), and then
          take the real connection that the superclass creates and wrap
          that connection in your MockConnection.

          So your connect() method should probably look something like:

          return new MockConnection(super.connect(url, info));

          and your MockConnection class will need to have a private
          internal Connection object that it delegates to.

          Does that make sense? Hopefully this helps you get further
          in building your test case.

          Show
          Bryan Pendleton added a comment - I modified the setUp() method of your patch so that it looks like this: protected void setUp() throws Exception { System.out.println("setUp is called!"); openDefaultConnection(); MockInternalDriver dvr = new MockInternalDriver(); dvr.stop(); dvr.boot(false, null); } and now the test gets considerably further. It gets to the point where it tries to make a new connection, and then fails. I think that your connect() method in your MockInternalDriver must do more than just return an empty MockConnection(). I think that you have to call super.connect(url, info), and then take the real connection that the superclass creates and wrap that connection in your MockConnection. So your connect() method should probably look something like: return new MockConnection(super.connect(url, info)); and your MockConnection class will need to have a private internal Connection object that it delegates to. Does that make sense? Hopefully this helps you get further in building your test case.
          Hide
          Houx Zhang added a comment -

          Thanks Bryan very much!

          I have tried following your advice, now, in the new patch, the MockInternalDriver can be instanced and its connect(String url, Properties info) is called indeed, however, its super.connect(url, info) throws a NullException. It seems this way is not OK. I still faced the same question--how to mock a rollback as expected.

          Wish for your comments!

          Show
          Houx Zhang added a comment - Thanks Bryan very much! I have tried following your advice, now, in the new patch, the MockInternalDriver can be instanced and its connect(String url, Properties info) is called indeed, however, its super.connect(url, info) throws a NullException. It seems this way is not OK. I still faced the same question--how to mock a rollback as expected. Wish for your comments!
          Hide
          Bryan Pendleton added a comment -

          Can you paste in the NPE exception that you get, with its stack trace?

          Show
          Bryan Pendleton added a comment - Can you paste in the NPE exception that you get, with its stack trace?
          Hide
          Houx Zhang added a comment -

          Certainly, Bryan.

          setUp is called!
          --------test--------
          MockInternalDriver.connect() is called!
          java.lang.NullPointerException
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.<init>(TransactionResourceImpl.java:161)
          at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.java:234)
          at org.apache.derby.impl.jdbc.EmbedConnection30.<init>(EmbedConnection30.java:73)
          at org.apache.derby.jdbc.Driver30.getNewEmbedConnection(Driver30.java:80)
          at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:248)
          at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest$MockInternalDriver.connect(RollBackWrappingWhenFailOnImportTest.java:56)
          at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:144)
          at java.sql.DriverManager.getConnection(DriverManager.java:582)
          at java.sql.DriverManager.getConnection(DriverManager.java:154)
          at org.apache.derbyTesting.junit.DriverManagerConnector.openConnection(DriverManagerConnector.java:81)
          at org.apache.derbyTesting.junit.DriverManagerConnector.openConnection(DriverManagerConnector.java:43)
          at org.apache.derbyTesting.junit.TestConfiguration.openDefaultConnection(TestConfiguration.java:1574)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.openDefaultConnection(BaseJDBCTestCase.java:428)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.getConnection(BaseJDBCTestCase.java:110)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.prepareCall(BaseJDBCTestCase.java:317)
          at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest.testRollBackWhenFailOnImportTable(RollBackWrappingWhenFailOnImportTest.java:90)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at junit.framework.TestCase.runTest(TestCase.java:164)
          at junit.framework.TestCase.runBare(TestCase.java:130)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112)
          at junit.framework.TestResult$1.protect(TestResult.java:106)
          at junit.framework.TestResult.runProtected(TestResult.java:124)
          at junit.framework.TestResult.run(TestResult.java:109)
          at junit.framework.TestCase.run(TestCase.java:120)
          at junit.framework.TestSuite.runTest(TestSuite.java:230)
          at junit.framework.TestSuite.run(TestSuite.java:225)
          at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
          at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

          Show
          Houx Zhang added a comment - Certainly, Bryan. setUp is called! -------- test -------- MockInternalDriver.connect() is called! java.lang.NullPointerException at org.apache.derby.impl.jdbc.TransactionResourceImpl.<init>(TransactionResourceImpl.java:161) at org.apache.derby.impl.jdbc.EmbedConnection.<init>(EmbedConnection.java:234) at org.apache.derby.impl.jdbc.EmbedConnection30.<init>(EmbedConnection30.java:73) at org.apache.derby.jdbc.Driver30.getNewEmbedConnection(Driver30.java:80) at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:248) at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest$MockInternalDriver.connect(RollBackWrappingWhenFailOnImportTest.java:56) at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:144) at java.sql.DriverManager.getConnection(DriverManager.java:582) at java.sql.DriverManager.getConnection(DriverManager.java:154) at org.apache.derbyTesting.junit.DriverManagerConnector.openConnection(DriverManagerConnector.java:81) at org.apache.derbyTesting.junit.DriverManagerConnector.openConnection(DriverManagerConnector.java:43) at org.apache.derbyTesting.junit.TestConfiguration.openDefaultConnection(TestConfiguration.java:1574) at org.apache.derbyTesting.junit.BaseJDBCTestCase.openDefaultConnection(BaseJDBCTestCase.java:428) at org.apache.derbyTesting.junit.BaseJDBCTestCase.getConnection(BaseJDBCTestCase.java:110) at org.apache.derbyTesting.junit.BaseJDBCTestCase.prepareCall(BaseJDBCTestCase.java:317) at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest.testRollBackWhenFailOnImportTable(RollBackWrappingWhenFailOnImportTest.java:90) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
          Hide
          Bryan Pendleton added a comment -

          I think that the suggestion I made in an earlier comment, about including the lines

          dvr.stop();
          dvr.boot(false, null);

          was not good advice. Those lines are what is causing your current NPE.

          I removed the calls to dvr.stop() and dvr.boot() from the setUp() method of
          the test, and I get much further.

          Can you try making that change in your copy, and see if that gets you further?

          Show
          Bryan Pendleton added a comment - I think that the suggestion I made in an earlier comment, about including the lines dvr.stop(); dvr.boot(false, null); was not good advice. Those lines are what is causing your current NPE. I removed the calls to dvr.stop() and dvr.boot() from the setUp() method of the test, and I get much further. Can you try making that change in your copy, and see if that gets you further?
          Hide
          Houx Zhang added a comment -

          Oh, good message, Bryan! Thanks to your patient advice, I think we have get closer to the aim.

          I have remove the lines

          dvr.stop();
          dvr.boot(false, null);
          , yes the test can run further, but the MockInternalDriver.connect() is not called nay more.

          If keep the "dvr.boot(false, null); ", we get what we want just like in patch-3, the MockConnection is created and rollBack() failed as expected.

          I have sumbitted the DERBY-4443-3.patch. Please check it. If it's OK, I will add more test cases, clean code and give a final version.

          Show
          Houx Zhang added a comment - Oh, good message, Bryan! Thanks to your patient advice, I think we have get closer to the aim. I have remove the lines dvr.stop(); dvr.boot(false, null); , yes the test can run further, but the MockInternalDriver.connect() is not called nay more. If keep the "dvr.boot(false, null); ", we get what we want just like in patch-3, the MockConnection is created and rollBack() failed as expected. I have sumbitted the DERBY-4443 -3.patch. Please check it. If it's OK, I will add more test cases, clean code and give a final version.
          Hide
          Bryan Pendleton added a comment -

          Excellent! I am pleased that this approach appears to work. I think that we will
          want to include some comments in the header of the new test to describe the
          use of the Mock testing objects, and to briefly outline the use of the 'dvr.boot'
          method to install our new mock driver into the test.

          I think you should proceed with this work; it will be great to have the fix, and
          even better that we'll have a regression test.

          Show
          Bryan Pendleton added a comment - Excellent! I am pleased that this approach appears to work. I think that we will want to include some comments in the header of the new test to describe the use of the Mock testing objects, and to briefly outline the use of the 'dvr.boot' method to install our new mock driver into the test. I think you should proceed with this work; it will be great to have the fix, and even better that we'll have a regression test.
          Hide
          Houx Zhang added a comment -

          Hi, Bryan. Thanks for your great encourage!

          I have finished the other test cases on importing, cleaned the code, added anotation. Please check it. Thanks again!

          Show
          Houx Zhang added a comment - Hi, Bryan. Thanks for your great encourage! I have finished the other test cases on importing, cleaned the code, added anotation. Please check it. Thanks again!
          Hide
          Bryan Pendleton added a comment -

          This latest patch is looking pretty good to me: the code changes look clear and
          straightforward, the test looks clear and to-the-point. The test passes for me
          with the patch applied; it also fails as I expect when the code change to
          SystemProjects.java is reverted, so the test seems to be testing the change
          successfully.

          Aaron, you mentioned in the original issue description that we might need to
          make this change in multiple places in Derby; are you aware of any places
          besides SystemProcedures.java where we issue unprotected rollback()
          calls like this?

          I suppose I'm trying to figure out whether this patch would resolve this issue
          completely, or whether it is a partial solution, and we will need to make
          similar changes in subsequent patches.

          Show
          Bryan Pendleton added a comment - This latest patch is looking pretty good to me: the code changes look clear and straightforward, the test looks clear and to-the-point. The test passes for me with the patch applied; it also fails as I expect when the code change to SystemProjects.java is reverted, so the test seems to be testing the change successfully. Aaron, you mentioned in the original issue description that we might need to make this change in multiple places in Derby; are you aware of any places besides SystemProcedures.java where we issue unprotected rollback() calls like this? I suppose I'm trying to figure out whether this patch would resolve this issue completely, or whether it is a partial solution, and we will need to make similar changes in subsequent patches.
          Hide
          Bryan Pendleton added a comment -

          When I run the new test as part of the complete JUnit suite, I see the following exception in my output:

          ..........java.sql.SQLException: Derby system shutdown.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:158)
          at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:222)
          at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:243)
          at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest$MockInternalDriver.connect(RollBackWrappingWhenFailOnImportTest.java:63)
          at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:144)
          at java.sql.DriverManager.getConnection(DriverManager.java:582)
          at java.sql.DriverManager.getConnection(DriverManager.java:154)
          at org.apache.derbyTesting.junit.DriverManagerConnector.getConnectionByAttributes(DriverManagerConnector.java:143)
          at org.apache.derbyTesting.junit.DriverManagerConnector.shutEngine(DriverManagerConnector.java:120)
          at org.apache.derbyTesting.junit.TestConfiguration.shutdownEngine(TestConfiguration.java:1635)
          at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.setUp(ErrorStreamTest.java:101)
          at junit.framework.TestCase.runBare(TestCase.java:125)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112)
          at junit.framework.TestResult$1.protect(TestResult.java:106)
          at junit.framework.TestResult.runProtected(TestResult.java:124)
          at junit.framework.TestResult.run(TestResult.java:109)
          at junit.framework.TestCase.run(TestCase.java:118)
          at junit.framework.TestSuite.runTest(TestSuite.java:208)
          at junit.framework.TestSuite.run(TestSuite.java:203)
          at junit.framework.TestSuite.runTest(TestSuite.java:208)
          at junit.framework.TestSuite.run(TestSuite.java:203)
          at junit.framework.TestSuite.runTest(TestSuite.java:208)
          at junit.framework.TestSuite.run(TestSuite.java:203)
          at junit.framework.TestSuite.runTest(TestSuite.java:208)
          at junit.framework.TestSuite.run(TestSuite.java:203)
          at junit.textui.TestRunner.doRun(TestRunner.java:116)
          at junit.textui.TestRunner.start(TestRunner.java:172)
          at junit.textui.TestRunner.main(TestRunner.java:138)
          Caused by: java.sql.SQLException: Derby system shutdown.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:122)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:71)
          ... 29 more
          E...............................

          I think that possibly the test needs to be run with a configuration that says it should only be
          run when we are using the embedded driver, not in the network server configuration?

          Show
          Bryan Pendleton added a comment - When I run the new test as part of the complete JUnit suite, I see the following exception in my output: ..........java.sql.SQLException: Derby system shutdown. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:158) at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:222) at org.apache.derby.jdbc.InternalDriver.connect(InternalDriver.java:243) at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest$MockInternalDriver.connect(RollBackWrappingWhenFailOnImportTest.java:63) at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:144) at java.sql.DriverManager.getConnection(DriverManager.java:582) at java.sql.DriverManager.getConnection(DriverManager.java:154) at org.apache.derbyTesting.junit.DriverManagerConnector.getConnectionByAttributes(DriverManagerConnector.java:143) at org.apache.derbyTesting.junit.DriverManagerConnector.shutEngine(DriverManagerConnector.java:120) at org.apache.derbyTesting.junit.TestConfiguration.shutdownEngine(TestConfiguration.java:1635) at org.apache.derbyTesting.functionTests.tests.engine.ErrorStreamTest.setUp(ErrorStreamTest.java:101) at junit.framework.TestCase.runBare(TestCase.java:125) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:118) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at junit.textui.TestRunner.doRun(TestRunner.java:116) at junit.textui.TestRunner.start(TestRunner.java:172) at junit.textui.TestRunner.main(TestRunner.java:138) Caused by: java.sql.SQLException: Derby system shutdown. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:122) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:71) ... 29 more E............................... I think that possibly the test needs to be run with a configuration that says it should only be run when we are using the embedded driver, not in the network server configuration?
          Hide
          Houx Zhang added a comment -

          It seems strange. How do your mean by "run the new test as part of the complete JUnit suite", Bryan?

          I have run the new TestCase both in Eclipse and DOS, they are all OK, just like shown in "Derby-4443-4.png". Even in tools._Suite, it can pass tool.

          Show
          Houx Zhang added a comment - It seems strange. How do your mean by "run the new test as part of the complete JUnit suite", Bryan? I have run the new TestCase both in Eclipse and DOS, they are all OK, just like shown in "Derby-4443-4.png". Even in tools._Suite, it can pass tool.
          Hide
          Bryan Pendleton added a comment -

          I did
          java -Xmx512m -XX:MaxPermSize=128m junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All >junitAll.out 2>&1

          and the exception was in the junitAll.out.

          Show
          Bryan Pendleton added a comment - I did java -Xmx512m -XX:MaxPermSize=128m junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All >junitAll.out 2>&1 and the exception was in the junitAll.out.
          Hide
          Knut Anders Hatlen added a comment -

          I agree that the approach looks fine. Some minor comments:

          • When calling printStackTrace() on the thrown exception, only the rollback exception is thrown. To see the real problem, you either need to call getNextException() or look in derby.log. Would it make sense to change the order here? That is, in rollBackAndThrowSQLException(), change

          e.setNextException(se);
          throw e;

          to

          se.setNextException(e);
          throw se;
          ?

          • There are three calls to printStackTrace() in the test code. It would probably be better to throw the exceptions so that JUnit can report them properly. If the exceptions are expected, it would be better to ignore them (with a comment saying why they should be ignored) or to have some asserts to verify that they are the exceptions we expect.
          Show
          Knut Anders Hatlen added a comment - I agree that the approach looks fine. Some minor comments: When calling printStackTrace() on the thrown exception, only the rollback exception is thrown. To see the real problem, you either need to call getNextException() or look in derby.log. Would it make sense to change the order here? That is, in rollBackAndThrowSQLException(), change e.setNextException(se); throw e; to se.setNextException(e); throw se; ? There are three calls to printStackTrace() in the test code. It would probably be better to throw the exceptions so that JUnit can report them properly. If the exceptions are expected, it would be better to ignore them (with a comment saying why they should be ignored) or to have some asserts to verify that they are the exceptions we expect.
          Hide
          Knut Anders Hatlen added a comment -

          One thing I forgot to mention... The test uses EmbedConnection30, so it won't work on platforms that don't support at least JDBC 3.0 (for example Java ME with the JSR-169 driver). So the suite() method will have to check JDBC.vmSupportsJDBC3() and return an empty suite if JDBC 3.0 isn't supported by the JVM.

          Show
          Knut Anders Hatlen added a comment - One thing I forgot to mention... The test uses EmbedConnection30, so it won't work on platforms that don't support at least JDBC 3.0 (for example Java ME with the JSR-169 driver). So the suite() method will have to check JDBC.vmSupportsJDBC3() and return an empty suite if JDBC 3.0 isn't supported by the JVM.
          Hide
          Houx Zhang added a comment -

          Thanks for your advice, Knut and Bryan. It's valuable for me to make progress.

          I have recreated the error with Bryan's command. Here is why it's caused:

          In the new test class 'RollBackWrappingWhenFailOnImportTest', a mock driver has been loaded, but it has not been shut down any more. In setup() method of engine.ErrorStreamTest which is closed in the completed test, a shutdownEngine() is called, it's used to shut down the current engine. The shutdown operation invokes a call to MockInternalDriver.connect(), which throw a mock connection(not valid here), so a SQLException of "Derby system shutdown" is thrown.

          It's obvious that, 'RollBackWrappingWhenFailOnImportTest' has made a marginal influence on other test cases, so in patch-5, I shut down the mock driver on tearDown(), -----the exception can be ignored safely. Now, everything works well.

          In patch-5, Knut's advices on printStackTrace() and vmSupportsJDBC3() are adopted. Thanks!

          Please check the new patch-5!

          Show
          Houx Zhang added a comment - Thanks for your advice, Knut and Bryan. It's valuable for me to make progress. I have recreated the error with Bryan's command. Here is why it's caused: In the new test class 'RollBackWrappingWhenFailOnImportTest', a mock driver has been loaded, but it has not been shut down any more. In setup() method of engine.ErrorStreamTest which is closed in the completed test, a shutdownEngine() is called, it's used to shut down the current engine. The shutdown operation invokes a call to MockInternalDriver.connect(), which throw a mock connection(not valid here), so a SQLException of "Derby system shutdown" is thrown. It's obvious that, 'RollBackWrappingWhenFailOnImportTest' has made a marginal influence on other test cases, so in patch-5, I shut down the mock driver on tearDown(), -----the exception can be ignored safely. Now, everything works well. In patch-5, Knut's advices on printStackTrace() and vmSupportsJDBC3() are adopted. Thanks! Please check the new patch-5!
          Hide
          Bryan Pendleton added a comment -

          The tests pass successfully in my environment now.

          I intend to commit this patch in the next few days, thanks for all the hard work on it!

          Show
          Bryan Pendleton added a comment - The tests pass successfully in my environment now. I intend to commit this patch in the next few days, thanks for all the hard work on it!
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for making these changes. I didn't see any response to my question about the order of the exception chain. If I print the exception thrown by rollBackAndThrowSQLException(), I see this:

          java.sql.SQLException: The exception 'java.sql.SQLException: error in roll back' was thrown while evaluating an expression.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.seeNextException(Util.java:278)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:348)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2290)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:82)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1334)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1686)
          at org.apache.derby.impl.jdbc.EmbedCallableStatement.executeStatement(EmbedCallableStatement.java:117)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:308)
          at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest.realTestRollBackWhenImportOnNonexistentFile(RollBackWrappingWhenFailOnImportTest.java:152)
          at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest.testRollBackWhenFailOnImportTable(RollBackWrappingWhenFailOnImportTest.java:119)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at junit.framework.TestCase.runTest(TestCase.java:168)
          at junit.framework.TestCase.runBare(TestCase.java:134)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112)
          at junit.framework.TestResult$1.protect(TestResult.java:110)
          at junit.framework.TestResult.runProtected(TestResult.java:128)
          at junit.framework.TestResult.run(TestResult.java:113)
          at junit.framework.TestCase.run(TestCase.java:124)
          at junit.framework.TestSuite.runTest(TestSuite.java:232)
          at junit.framework.TestSuite.run(TestSuite.java:227)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:23)
          at junit.framework.TestResult.runProtected(TestResult.java:128)
          at junit.extensions.TestSetup.run(TestSetup.java:27)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.framework.TestSuite.runTest(TestSuite.java:232)
          at junit.framework.TestSuite.run(TestSuite.java:227)
          at junit.textui.TestRunner.doRun(TestRunner.java:116)
          at junit.textui.TestRunner.start(TestRunner.java:180)
          at junit.textui.TestRunner.main(TestRunner.java:138)
          Caused by: java.sql.SQLException: The exception 'java.sql.SQLException: error in roll back' was thrown while evaluating an expression.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:122)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:71)
          ... 35 more
          Caused by: java.sql.SQLException: error in roll back
          at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest$MockInternalDriver$MockConnectionFailWhenRollBack.rollback(RollBackWrappingWhenFailOnImportTest.java:59)
          at org.apache.derby.catalog.SystemProcedures.rollBackAndThrowSQLException(SystemProcedures.java:1479)
          at org.apache.derby.catalog.SystemProcedures.SYSCS_IMPORT_TABLE(SystemProcedures.java:1466)
          at org.apache.derby.exe.aceaa980c4x012fx0589x77eaxffffbc38504a0.g0(Unknown Source)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at org.apache.derby.impl.services.reflect.ReflectMethod.invoke(ReflectMethod.java:46)
          at org.apache.derby.impl.sql.execute.CallStatementResultSet.open(CallStatementResultSet.java:75)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436)
          at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1242)
          ... 28 more

          For a naive user, this looks like a problem in rollback(), like it did before the fix, and there's no trace of the original "table not found" exception without doing some digging in the logs or traversing the getNextException() chain. Any thoughts on whether this is the exception we should report as the primary exception?

          Show
          Knut Anders Hatlen added a comment - Thanks for making these changes. I didn't see any response to my question about the order of the exception chain. If I print the exception thrown by rollBackAndThrowSQLException(), I see this: java.sql.SQLException: The exception 'java.sql.SQLException: error in roll back' was thrown while evaluating an expression. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:98) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.seeNextException(Util.java:278) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:348) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2290) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:82) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1334) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1686) at org.apache.derby.impl.jdbc.EmbedCallableStatement.executeStatement(EmbedCallableStatement.java:117) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:308) at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest.realTestRollBackWhenImportOnNonexistentFile(RollBackWrappingWhenFailOnImportTest.java:152) at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest.testRollBackWhenFailOnImportTable(RollBackWrappingWhenFailOnImportTest.java:119) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:168) at junit.framework.TestCase.runBare(TestCase.java:134) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:124) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:23) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.extensions.TestSetup.run(TestSetup.java:27) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at junit.textui.TestRunner.doRun(TestRunner.java:116) at junit.textui.TestRunner.start(TestRunner.java:180) at junit.textui.TestRunner.main(TestRunner.java:138) Caused by: java.sql.SQLException: The exception 'java.sql.SQLException: error in roll back' was thrown while evaluating an expression. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:122) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:71) ... 35 more Caused by: java.sql.SQLException: error in roll back at org.apache.derbyTesting.functionTests.tests.tools.RollBackWrappingWhenFailOnImportTest$MockInternalDriver$MockConnectionFailWhenRollBack.rollback(RollBackWrappingWhenFailOnImportTest.java:59) at org.apache.derby.catalog.SystemProcedures.rollBackAndThrowSQLException(SystemProcedures.java:1479) at org.apache.derby.catalog.SystemProcedures.SYSCS_IMPORT_TABLE(SystemProcedures.java:1466) at org.apache.derby.exe.aceaa980c4x012fx0589x77eaxffffbc38504a0.g0(Unknown Source) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.apache.derby.impl.services.reflect.ReflectMethod.invoke(ReflectMethod.java:46) at org.apache.derby.impl.sql.execute.CallStatementResultSet.open(CallStatementResultSet.java:75) at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436) at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1242) ... 28 more For a naive user, this looks like a problem in rollback(), like it did before the fix, and there's no trace of the original "table not found" exception without doing some digging in the logs or traversing the getNextException() chain. Any thoughts on whether this is the exception we should report as the primary exception?
          Hide
          Bryan Pendleton added a comment -

          Hi Knut Anders, thanks for double-checking that.

          I agree that we would prefer to have the original exception lead the stack
          trace, with the rollback exception chained on afterwards.

          I made your suggested change to rollbackAndThrowSQLException,
          and indeed the exception in the stack trace is more relevant to the
          original problem.

          However, this will require adjusting the test, since now the stack trace
          looks different to the assertions in the test code.

          I'll hold off on committing while we discuss this further.

          Show
          Bryan Pendleton added a comment - Hi Knut Anders, thanks for double-checking that. I agree that we would prefer to have the original exception lead the stack trace, with the rollback exception chained on afterwards. I made your suggested change to rollbackAndThrowSQLException, and indeed the exception in the stack trace is more relevant to the original problem. However, this will require adjusting the test, since now the stack trace looks different to the assertions in the test code. I'll hold off on committing while we discuss this further.
          Hide
          Houx Zhang added a comment -

          Thanks for your checking, Knut.

          This problem has perplexed me for some days. After checking again, I accept that

          private static void rollBackAndThrowSQLException(Connection conn,
          SQLException se) throws SQLException {
          try

          { conn.rollback(); }

          catch (SQLException e)

          { se.setNextException(e); }

          throw se;
          }

          is better, we will get the full SQLException chain. However, it's very strange that when debugging in this situation, the chain will be broken down before thrown to EmbedPreparedStatement.executeUpdate() in Line 309 of EmbedPreparedStatement, and the rollback exception will be lost.

          Is this situation normal, or is there something special for SQLException launched by rollback(), please?

          Show
          Houx Zhang added a comment - Thanks for your checking, Knut. This problem has perplexed me for some days. After checking again, I accept that private static void rollBackAndThrowSQLException(Connection conn, SQLException se) throws SQLException { try { conn.rollback(); } catch (SQLException e) { se.setNextException(e); } throw se; } is better, we will get the full SQLException chain. However, it's very strange that when debugging in this situation, the chain will be broken down before thrown to EmbedPreparedStatement.executeUpdate() in Line 309 of EmbedPreparedStatement, and the rollback exception will be lost. Is this situation normal, or is there something special for SQLException launched by rollback(), please?
          Hide
          Knut Anders Hatlen added a comment -

          Does the rollback exception appear if you call getNextException() on the thrown SQLException? You may need to call getNextException() multiple times before you get to the rollback exception. Derby typically adds a next exception for all errors, so the chain may consist of more than two exceptions in this case.

          Show
          Knut Anders Hatlen added a comment - Does the rollback exception appear if you call getNextException() on the thrown SQLException? You may need to call getNextException() multiple times before you get to the rollback exception. Derby typically adds a next exception for all errors, so the chain may consist of more than two exceptions in this case.
          Hide
          Houx Zhang added a comment -

          Knut, I have called getNextException() in several places, such as EmbedPreparedStatement.executeUpdate() in Line 309 of EmbedPreparedStatement, and each of they returned null. I also looked into the trace stack when debugging, and haven't seen rollback exception in it.

          Maybe it relates to the simple mocking of InternalDriver and Connection?

          Show
          Houx Zhang added a comment - Knut, I have called getNextException() in several places, such as EmbedPreparedStatement.executeUpdate() in Line 309 of EmbedPreparedStatement, and each of they returned null. I also looked into the trace stack when debugging, and haven't seen rollback exception in it. Maybe it relates to the simple mocking of InternalDriver and Connection?
          Hide
          Knut Anders Hatlen added a comment -

          It looks like the rollback exception is stripped off by this piece of code in StandardException.unexpectedUserException():

          // Look for simple wrappers for 3.0.1 - will be cleaned up in main
          if (ferry != null) {
          if (ferry.isSimpleWrapper())

          { Throwable wrapped = ((SQLException)ferry).getCause(); if (wrapped instanceof StandardException) return (StandardException) wrapped; }

          }

          This happens when the SQLException needs to be re-thrown as a StandardException, and the error handler finds that it's simpler to remove the top-level SQLException than it is to wrap it in yet another exception. Normally, an SQLException that wraps a StandardException doesn't provide any extra information, so removing it doesn't harm. But in our case the top-level exception carries extra information (the next exception), and it's not really a simple wrapper anymore. Perhaps we need to override setNextException() in EmbedSQLException and clear the simpleWrapper flag if it's called. The comments in the StandardException class give the impression that this "simple wrapper" code was a temporary hack that was supposed to be reworked, but they don't give any details on how.

          Show
          Knut Anders Hatlen added a comment - It looks like the rollback exception is stripped off by this piece of code in StandardException.unexpectedUserException(): // Look for simple wrappers for 3.0.1 - will be cleaned up in main if (ferry != null) { if (ferry.isSimpleWrapper()) { Throwable wrapped = ((SQLException)ferry).getCause(); if (wrapped instanceof StandardException) return (StandardException) wrapped; } } This happens when the SQLException needs to be re-thrown as a StandardException, and the error handler finds that it's simpler to remove the top-level SQLException than it is to wrap it in yet another exception. Normally, an SQLException that wraps a StandardException doesn't provide any extra information, so removing it doesn't harm. But in our case the top-level exception carries extra information (the next exception), and it's not really a simple wrapper anymore. Perhaps we need to override setNextException() in EmbedSQLException and clear the simpleWrapper flag if it's called. The comments in the StandardException class give the impression that this "simple wrapper" code was a temporary hack that was supposed to be reworked, but they don't give any details on how.
          Hide
          Bryan Pendleton added a comment -

          Would it help if we made rollBackAndThrowSQLException throw
          something other than a SQLException when it caught a rollback() exception?

          For example, would it help if that catch() block threw a StandardException instead?

          I guess I'm not quite sure I understand what rule StandardException is
          enforcing. Is it OK for a StandardException to wrap a StandardException, but
          if a SQLException wraps a StandardException than it gets automatically unwrapped?

          I suspect this has to do with improving the external user experience for the
          caller; particularly in client/server scenarios the exceptions that travel from
          server to client tend to get wrapped along the way with uninteresting and not-so-useful
          outer exceptions, and so perhaps this code is trying to undo that and get
          more quickly to the underlying exception since it is more likely to reveal the real problem.

          Show
          Bryan Pendleton added a comment - Would it help if we made rollBackAndThrowSQLException throw something other than a SQLException when it caught a rollback() exception? For example, would it help if that catch() block threw a StandardException instead? I guess I'm not quite sure I understand what rule StandardException is enforcing. Is it OK for a StandardException to wrap a StandardException, but if a SQLException wraps a StandardException than it gets automatically unwrapped? I suspect this has to do with improving the external user experience for the caller; particularly in client/server scenarios the exceptions that travel from server to client tend to get wrapped along the way with uninteresting and not-so-useful outer exceptions, and so perhaps this code is trying to undo that and get more quickly to the underlying exception since it is more likely to reveal the real problem.
          Hide
          Knut Anders Hatlen added a comment -

          > I guess I'm not quite sure I understand what rule StandardException is
          > enforcing. Is it OK for a StandardException to wrap a StandardException, but
          > if a SQLException wraps a StandardException than it gets automatically unwrapped?

          It typically only gets unwrapped if it has been thrown inside a stored
          procedure (only exceptions generated by
          PublicAPI.wrapStandardException() are marked as "simple wrappers").
          What it attempts to solve is the problem of passing exceptions through
          multiple layers. Inside the language layer and store, exceptions have
          to be passed as StandardExceptions, whereas in the JDBC layer they
          have to be passed as SQLExceptions.

          In the common case, an exception is generated inside the engine as a
          StandardException, and it gets wrapped in an SQLException on its way
          out through the JDBC layer. But in the case of stored procedures, the
          JDBC layer is reentered from the language layer. The exceptions that
          happen inside the stored procedure are returned to the language layer
          as SQLExceptions, but have to be transported as StandardExceptions
          through the language layer up to the outer JDBC layer. This could be
          done by wrapping the SQLException in a StandardException, but then the
          end result would be an SQLException wrapping a StandardException
          wrapping an SQLException wrapping a StandardException. By just passing
          the underlying StandardException, we save two layers of indirection.

          Show
          Knut Anders Hatlen added a comment - > I guess I'm not quite sure I understand what rule StandardException is > enforcing. Is it OK for a StandardException to wrap a StandardException, but > if a SQLException wraps a StandardException than it gets automatically unwrapped? It typically only gets unwrapped if it has been thrown inside a stored procedure (only exceptions generated by PublicAPI.wrapStandardException() are marked as "simple wrappers"). What it attempts to solve is the problem of passing exceptions through multiple layers. Inside the language layer and store, exceptions have to be passed as StandardExceptions, whereas in the JDBC layer they have to be passed as SQLExceptions. In the common case, an exception is generated inside the engine as a StandardException, and it gets wrapped in an SQLException on its way out through the JDBC layer. But in the case of stored procedures, the JDBC layer is reentered from the language layer. The exceptions that happen inside the stored procedure are returned to the language layer as SQLExceptions, but have to be transported as StandardExceptions through the language layer up to the outer JDBC layer. This could be done by wrapping the SQLException in a StandardException, but then the end result would be an SQLException wrapping a StandardException wrapping an SQLException wrapping a StandardException. By just passing the underlying StandardException, we save two layers of indirection.
          Hide
          Houx Zhang added a comment -

          Hello, I have provided DERBY-4443-6-tmp.patch.

          private static void rollBackAndThrowSQLException(Connection conn,
          SQLException se) throws SQLException {
          try

          { conn.rollback(); }

          catch (SQLException e)

          { se.setNextException(e); }

          StandardException ste = StandardException.newException(
          se.getSQLState(), (Throwable)se);
          SQLException sqle = new SQLException((Throwable)ste); //this line can not pass compilation

          throw sqle;
          }

          In rollBackAndThrowSQLException() of this patch, SQLException has been wrapped in StandardException, which is wrapped in another SQLException. However, the patch can not pass compilation, and says "constructor SQLException(java.lang.Throwable) can not be found". I'm puzzled about it. Please give an advice, thanks!

          Show
          Houx Zhang added a comment - Hello, I have provided DERBY-4443 -6-tmp.patch. private static void rollBackAndThrowSQLException(Connection conn, SQLException se) throws SQLException { try { conn.rollback(); } catch (SQLException e) { se.setNextException(e); } StandardException ste = StandardException.newException( se.getSQLState(), (Throwable)se); SQLException sqle = new SQLException((Throwable)ste); //this line can not pass compilation throw sqle; } In rollBackAndThrowSQLException() of this patch, SQLException has been wrapped in StandardException, which is wrapped in another SQLException. However, the patch can not pass compilation, and says "constructor SQLException(java.lang.Throwable) can not be found". I'm puzzled about it. Please give an advice, thanks!
          Hide
          Bryan Pendleton added a comment -

          I think that the compilation error is because this particular class (SystemProcedures.java) is
          compiled against the JSR169 version of SQLException, which you can find in
          ./java/stubs/jsr169/java/sql/SQLException.java, and that version of SQLException does
          not have a constructor which takes a Throwable.

          What did you think about Knut Anders's suggestion of going back to your earlier patch,
          and overriding setNextException in EmbedSQLException to clear the simpleWrapper flag?

          Show
          Bryan Pendleton added a comment - I think that the compilation error is because this particular class (SystemProcedures.java) is compiled against the JSR169 version of SQLException, which you can find in ./java/stubs/jsr169/java/sql/SQLException.java, and that version of SQLException does not have a constructor which takes a Throwable. What did you think about Knut Anders's suggestion of going back to your earlier patch, and overriding setNextException in EmbedSQLException to clear the simpleWrapper flag?
          Hide
          Houx Zhang added a comment -

          Hi, Bryan, thanks for your reminding, I think the new patch 4443-7 can work well now. Please check it.

          Show
          Houx Zhang added a comment - Hi, Bryan, thanks for your reminding, I think the new patch 4443-7 can work well now. Please check it.
          Hide
          Bryan Pendleton added a comment -

          Patch 7 builds fine for me and the new test passes cleanly. I also verified that if
          I take out the change to EmbedSQLException, the test then fails with the expected
          failure, so I think this is looking very good.

          Houx, have you run the complete test suite with this patch? Or have you just run the new test?

          Show
          Bryan Pendleton added a comment - Patch 7 builds fine for me and the new test passes cleanly. I also verified that if I take out the change to EmbedSQLException, the test then fails with the expected failure, so I think this is looking very good. Houx, have you run the complete test suite with this patch? Or have you just run the new test?
          Hide
          Houx Zhang added a comment -

          Hi, Bryan. I have only run the tools.Suite, but not suites.All.

          After saw your adivce, I have run the complete test (org.apache.derbyTesting.functionTests.suites.All. Is that right ?) sometimes for several times, as been broken down by accident. The complete test suite takes me more than 5 hours for one run.

          I have seen the result containing some failures, but after run the complete test suite and dividual tests on the original state, I believe they are not caused by the new patch. So the new patch is OK.

          Show
          Houx Zhang added a comment - Hi, Bryan. I have only run the tools.Suite, but not suites.All. After saw your adivce, I have run the complete test (org.apache.derbyTesting.functionTests.suites.All. Is that right ?) sometimes for several times, as been broken down by accident. The complete test suite takes me more than 5 hours for one run. I have seen the result containing some failures, but after run the complete test suite and dividual tests on the original state, I believe they are not caused by the new patch. So the new patch is OK.
          Hide
          Bryan Pendleton added a comment -

          Thank you Houx for the update.

          I understand about the tests; they take a very long time for me, too. I am hoping to
          set up a faster machine for running the tests sometime over the summer, but for
          now I am like you, it is quite hard to run the tests. So I don't run the full suite very
          often, but I do like to run it on a patch whenever I can.

          I am going to move ahead with committing patch 7.

          Show
          Bryan Pendleton added a comment - Thank you Houx for the update. I understand about the tests; they take a very long time for me, too. I am hoping to set up a faster machine for running the tests sometime over the summer, but for now I am like you, it is quite hard to run the tests. So I don't run the full suite very often, but I do like to run it on a patch whenever I can. I am going to move ahead with committing patch 7.
          Hide
          Bryan Pendleton added a comment -

          I committed patch 7 to the trunk as revision 1096991.

          Thanks for all the hard work on this change; the testing was
          a bit tricky but I'm pleased that we worked through the issues
          to get a test case that solidly demonstrates the new functionality.

          Show
          Bryan Pendleton added a comment - I committed patch 7 to the trunk as revision 1096991. Thanks for all the hard work on this change; the testing was a bit tricky but I'm pleased that we worked through the issues to get a test case that solidly demonstrates the new functionality.
          Hide
          Houx Zhang added a comment -

          Hehe, I have harvested a lot on this issue, thanks very much for your and Knut's patient help!

          Show
          Houx Zhang added a comment - Hehe, I have harvested a lot on this issue, thanks very much for your and Knut's patient help!
          Hide
          Myrna van Lunteren added a comment -

          reopening for backport to 10.8

          Show
          Myrna van Lunteren added a comment - reopening for backport to 10.8
          Hide
          Myrna van Lunteren added a comment -

          committed backport to 10.8 with revision 1161226.

          Show
          Myrna van Lunteren added a comment - committed backport to 10.8 with revision 1161226.
          Hide
          Kathey Marsden added a comment -

          Reopen for 10.5 backport consideration. If working on the backport for this issue. Temporarily assign yourself and add a comment that you are working on it. When finished, reresolve with the new fix versions or label backport_reject_10_x as appropriate.

          Show
          Kathey Marsden added a comment - Reopen for 10.5 backport consideration. If working on the backport for this issue. Temporarily assign yourself and add a comment that you are working on it. When finished, reresolve with the new fix versions or label backport_reject_10_x as appropriate.
          Hide
          Myrna van Lunteren added a comment -

          I'll attempt backport of this to 10.5.
          Temporarily assigned to me.

          Show
          Myrna van Lunteren added a comment - I'll attempt backport of this to 10.5. Temporarily assigned to me.
          Hide
          Myrna van Lunteren added a comment -

          backported to 10.7 with revision 1304630, to 10.6 with revision 1304632, and to 10.5 with revision 1304636. Resetting assignee and closing again.

          Show
          Myrna van Lunteren added a comment - backported to 10.7 with revision 1304630, to 10.6 with revision 1304632, and to 10.5 with revision 1304636. Resetting assignee and closing again.

            People

            • Assignee:
              Houx Zhang
              Reporter:
              Aaron Digulla
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development