Derby
  1. Derby
  2. DERBY-5912

testIsValidImplemented fails for NetworkServer in some slow running machines/configurations

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.8.3.0
    • Fix Version/s: 10.8.3.0, 10.9.2.2, 10.10.1.1
    • Component/s: Test
    • Labels:
      None
    • Bug behavior facts:
      Regression Test Failure

      Description

      The following test has been seen to fail as below in some runs where the machine is under heavy load and slow running options are specified and the isValid() call takes more than a second to return.

      1) testIsValidImplemented(org.apache.derbyTesting.functionTests.tests.jdbc4.ConnectionTest)junit.framework.AssertionFailedError
      at org.apache.derbyTesting.functionTests.tests.jdbc4.ConnectionTest.testIsValidImplemented(ConnectionTest.java:168)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:60)
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:37)
      at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:113)
      at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
      at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
      at junit.extensions.TestSetup.run(TestSetup.java:23)
      at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
      at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
      at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
      at junit.extensions.TestSetup.run(TestSetup.java:23)

      The test does:
      // Test with a 1 second timeout
      assertTrue(getConnection().isValid(1));

      assuming it will return in one second. For embedded the int parameter is not implemented so indeed this always passes. For the Network implementation in NetConnection40.java we actually do timeout and perform a query as part of the implementation so might indeed return false.

      1. DERBY-5912_2.diff
        4 kB
        Myrna van Lunteren
      2. DERBY-5912.diff
        0.8 kB
        Myrna van Lunteren

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
          Hide
          Myrna van Lunteren added a comment -

          Backported the comment change, and the change to ConnectionMethodsTest to 10.9 with revision 1384397 (http://svn.apache.org/viewvc?view=revision&revision=1384397), and to 10.8 with revision 1384432 (http://svn.apache.org/viewvc?view=revision&revision=1384432).

          Show
          Myrna van Lunteren added a comment - Backported the comment change, and the change to ConnectionMethodsTest to 10.9 with revision 1384397 ( http://svn.apache.org/viewvc?view=revision&revision=1384397 ), and to 10.8 with revision 1384432 ( http://svn.apache.org/viewvc?view=revision&revision=1384432 ).
          Hide
          Myrna van Lunteren added a comment -

          committed with revision: http://svn.apache.org/viewvc?view=revision&revision=1381731
          The change to ConnectionMethodsTest can be backported to 10.9 and 10.8 if needed (and the name correction in the comment in ConnectionTest) .

          Show
          Myrna van Lunteren added a comment - committed with revision: http://svn.apache.org/viewvc?view=revision&revision=1381731 The change to ConnectionMethodsTest can be backported to 10.9 and 10.8 if needed (and the name correction in the comment in ConnectionTest) .
          Hide
          Myrna van Lunteren added a comment -

          Attaching a patch that:

          • changes the timeout in ConnectionMethodsTest from isValid(1) to isValid(200) and adds a comment
          • updates the comment in ConnectionTest so it refers to ConnectionMethodsTest
          • has commented out sleep code in DRDAConnThread that can be uncommented to test this
          • adds a fixture, testIsValidWithTimeout, that is disabled by having an 'x' in front of it, to ConnectionTest.
          Show
          Myrna van Lunteren added a comment - Attaching a patch that: changes the timeout in ConnectionMethodsTest from isValid(1) to isValid(200) and adds a comment updates the comment in ConnectionTest so it refers to ConnectionMethodsTest has commented out sleep code in DRDAConnThread that can be uncommented to test this adds a fixture, testIsValidWithTimeout, that is disabled by having an 'x' in front of it, to ConnectionTest.
          Hide
          Myrna van Lunteren added a comment -

          Kathey was right, when I tested this manually the connection following the timeout indeed had become unavailable. I logged DERBY-5919 for this.

          The location of the sleep in EXCSQLIMM was not right, because the isValid call is actually a prepared statement call, see NetConnection40:
          "...
          // Set the query timeout
          isValidStmt.setQueryTimeout(timeout);

          // Run the query against the database
          ResultSet rs = isValidStmt.executeQuery();
          rs.close();
          ... "

          After some discussion with Kathey, I first tried the sleep in CodePoint.PRPSQLSTT, but that wasn't correct either, because before the above code snippet, NetConnection40 has this:

          "....
          if (isValidStmt == null)

          { isValidStmt = prepareStatement("VALUES (1)"); }


          ..."

          Kathey said: So the prepareStatment() won't happen the second time and so we won't go through PRPSQLSTT on the server.
          The right location for the sleep is in CodePoint.OPNQRY.

          I modified the current test with a test case that will not run unless one removes a 'x' in front of the fixture name, and added the sleep, but commented out, into DRDAConnThread.

          I also noticed the comment saying that isValid was also tested in TestConnectionMethods.java.
          That test has been converted to junit and renamed ConnectionMethodsTest, so I updated the comment.
          I also modified ConnectionMethodsTest to have a longer timeout.

          I will attach these changes as patch, then commit these changes shortly.

          Show
          Myrna van Lunteren added a comment - Kathey was right, when I tested this manually the connection following the timeout indeed had become unavailable. I logged DERBY-5919 for this. The location of the sleep in EXCSQLIMM was not right, because the isValid call is actually a prepared statement call, see NetConnection40: "... // Set the query timeout isValidStmt.setQueryTimeout(timeout); // Run the query against the database ResultSet rs = isValidStmt.executeQuery(); rs.close(); ... " After some discussion with Kathey, I first tried the sleep in CodePoint.PRPSQLSTT, but that wasn't correct either, because before the above code snippet, NetConnection40 has this: ".... if (isValidStmt == null) { isValidStmt = prepareStatement("VALUES (1)"); } ..." Kathey said: So the prepareStatment() won't happen the second time and so we won't go through PRPSQLSTT on the server. The right location for the sleep is in CodePoint.OPNQRY. I modified the current test with a test case that will not run unless one removes a 'x' in front of the fixture name, and added the sleep, but commented out, into DRDAConnThread. I also noticed the comment saying that isValid was also tested in TestConnectionMethods.java. That test has been converted to junit and renamed ConnectionMethodsTest, so I updated the comment. I also modified ConnectionMethodsTest to have a longer timeout. I will attach these changes as patch, then commit these changes shortly.
          Hide
          Kathey Marsden added a comment -

          I could not think of an automated way to to test the timeout, but it occurred to me that it would be worth testing manually because I think maybe the current client code would actually cause the connection to become invalid if it timed out because socket_.setSoTimeout would cause permanent loss of the connection if it timed out. There may be some recovery mechanism that I am missing though. I think you can force a timeout by putting a sleep in DRDAConnThread.processCommands() right after this code:
          case CodePoint.EXCSQLIMM:
          try {

          Show
          Kathey Marsden added a comment - I could not think of an automated way to to test the timeout, but it occurred to me that it would be worth testing manually because I think maybe the current client code would actually cause the connection to become invalid if it timed out because socket_.setSoTimeout would cause permanent loss of the connection if it timed out. There may be some recovery mechanism that I am missing though. I think you can force a timeout by putting a sleep in DRDAConnThread.processCommands() right after this code: case CodePoint.EXCSQLIMM: try {
          Hide
          Myrna van Lunteren added a comment -

          backported the fix to 10.9 with revision http://svn.apache.org/viewvc?view=revision&revision=1379219 and backported that to 10.8 with revision http://svn.apache.org/viewvc?view=revision&revision=1379230.

          Resolving - if we think of an automated way to test the timeout we can open a new issue for that specifically.

          Show
          Myrna van Lunteren added a comment - backported the fix to 10.9 with revision http://svn.apache.org/viewvc?view=revision&revision=1379219 and backported that to 10.8 with revision http://svn.apache.org/viewvc?view=revision&revision=1379230 . Resolving - if we think of an automated way to test the timeout we can open a new issue for that specifically.
          Hide
          Myrna van Lunteren added a comment -
          Show
          Myrna van Lunteren added a comment - I committed the change with revision http://svn.apache.org/viewvc?view=revision&revision=1379163
          Hide
          Myrna van Lunteren added a comment -

          Attaching a patch which basically just sets a -arbitrarily chosen - really long timeout. For most machines this shouldn't matter as it normally doesn't time out.

          Looks from the comments in DERBY-1090 that Olav tested the time-out by adding a 'sleep' somewhere in the network server code. Does anyone have any suggestions on how to cause a time-out for this case so it can be tested for?

          Show
          Myrna van Lunteren added a comment - Attaching a patch which basically just sets a -arbitrarily chosen - really long timeout. For most machines this shouldn't matter as it normally doesn't time out. Looks from the comments in DERBY-1090 that Olav tested the time-out by adding a 'sleep' somewhere in the network server code. Does anyone have any suggestions on how to cause a time-out for this case so it can be tested for?
          Hide
          Myrna van Lunteren added a comment -

          Linking to DERBY-1090, which has a discussion about the implementation of this method.

          Show
          Myrna van Lunteren added a comment - Linking to DERBY-1090 , which has a discussion about the implementation of this method.

            People

            • Assignee:
              Myrna van Lunteren
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development