Derby
  1. Derby
  2. DERBY-5427

Unauthorized shutdown should not generate thread dump and javacore. AuthenticationTest dumps over 20 javacores with IBM JVM for normal user errors

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.4
    • Fix Version/s: 10.8.3.0, 10.9.1.0
    • Component/s: Services
    • Labels:
      None
    • Urgency:
      Urgent
    • Issue & fix info:
      High Value Fix, Patch Available, Repro attached, Workaround attached
    • Bug behavior facts:
      Regression

      Description

      If jdbcapi.AuthenticationTest is run without changing the derby.stream.error.extendedDiagSeverityLevel as is done in the test, it generates thread dumps and javacores for IBM jvms. I beleive the errors in this test are expected user errors and not crashes so they should not generate a thread dump or javacore. To reproduce remove this line from test:

      //Derby-4856,set the property to avoid thread dump and diagnostic info
      sysprops.put("derby.stream.error.extendedDiagSeverityLevel","50000");

      Until fixed, users can work around the issue by setting derby.stream.error.extendedDiagSeverityLevel to 50000 as is done in the test.

      1. derby-5427_diff.txt
        2 kB
        Kathey Marsden
      2. derby-5427_test_diff.txt
        0.9 kB
        Kathey Marsden

        Issue Links

          Activity

          Hide
          Lily Wei added a comment -

          The test generates more than 20 unauthorized shutdown errors. The numbers of javacore files depend on number of the same unauthorized shutdown error. To fix this from error point of view, we need to increase the error level for “ERROR 08004: Database connection refused”. As fix for DERBY-4856, we did not decide to change the error level for ERROR 08004. This is a good time to revisit this issue. Should we increase the error level for ERROR 08004 to be higher to avoid the javacore for IBM JVM instead of the workaround fix by setting derby.stream.error.extendedDiagSeverityLevel to 50000?

          Show
          Lily Wei added a comment - The test generates more than 20 unauthorized shutdown errors. The numbers of javacore files depend on number of the same unauthorized shutdown error. To fix this from error point of view, we need to increase the error level for “ERROR 08004: Database connection refused”. As fix for DERBY-4856 , we did not decide to change the error level for ERROR 08004. This is a good time to revisit this issue. Should we increase the error level for ERROR 08004 to be higher to avoid the javacore for IBM JVM instead of the workaround fix by setting derby.stream.error.extendedDiagSeverityLevel to 50000?
          Hide
          Kathey Marsden added a comment -

          On a simple functional level, the task of DERBY-4586 was to generate a thread dump/javacore when Derby crashes. This is not a crash so should not generate a javacore, so users and the tests should not have to work around generating javacores in this situation by setting derby.stream.error.extendedDiagSeverityLevel ,

          8004 is currently a session level error which seems correct to me.

          So I think the problem here is that our determination of whether this situation is a a crash is a bit off.
          As I recall the determination of a crash was when we got a session level error on a booted database. Perhaps it should be instead if we get a session level error on an active session.

          Show
          Kathey Marsden added a comment - On a simple functional level, the task of DERBY-4586 was to generate a thread dump/javacore when Derby crashes. This is not a crash so should not generate a javacore, so users and the tests should not have to work around generating javacores in this situation by setting derby.stream.error.extendedDiagSeverityLevel , 8004 is currently a session level error which seems correct to me. So I think the problem here is that our determination of whether this situation is a a crash is a bit off. As I recall the determination of a crash was when we got a session level error on a booted database. Perhaps it should be instead if we get a session level error on an active session.
          Hide
          Lily Wei added a comment -

          Currently, we are using whether a booted database get a session level error to determine whether to generate a thread dump/javacore. Adding the logic on top of that with whether it is a active session sounds more accurate to determine a Derby crash situation occur or not. In AuthenticationTest case, Derby is not crashing. If we add whether it is a active session logic to the code, it should be closer for the fix of DERBY-4586 with the goal to detecting Derby is crashing. Can anyone suggest how to determine whether we have a active session or not?

          Show
          Lily Wei added a comment - Currently, we are using whether a booted database get a session level error to determine whether to generate a thread dump/javacore. Adding the logic on top of that with whether it is a active session sounds more accurate to determine a Derby crash situation occur or not. In AuthenticationTest case, Derby is not crashing. If we add whether it is a active session logic to the code, it should be closer for the fix of DERBY-4586 with the goal to detecting Derby is crashing. Can anyone suggest how to determine whether we have a active session or not?
          Hide
          Kathey Marsden added a comment -

          I verified that this is also an issue with 10.8.1.2 as DERBY-4586 went into that release.

          I am finding it a bit difficult to get a test case outside of AuthenticationTest. Simple authentication failures with ij do not cause the issue, so may be more of an edge case than first thought, but I will continue to debug.

          I think it is an important issue, especially for IBM jvm's as leads to the javacore in addition to the thread dump in the derby.log that would print with other JVM's, but I don't think it needs to hold up the release.

          Show
          Kathey Marsden added a comment - I verified that this is also an issue with 10.8.1.2 as DERBY-4586 went into that release. I am finding it a bit difficult to get a test case outside of AuthenticationTest. Simple authentication failures with ij do not cause the issue, so may be more of an edge case than first thought, but I will continue to debug. I think it is an important issue, especially for IBM jvm's as leads to the javacore in addition to the thread dump in the derby.log that would print with other JVM's, but I don't think it needs to hold up the release.
          Hide
          Kathey Marsden added a comment -

          Attached is a patch which fixes this issue, but I don't know if it is the best solution. The critieria for a crash needs to be that we do the thread dump if we get a session level error while the session is active. To test whether the session was active we would just check database.isActive(). In the problematic authentication cases, database.isActive() was true and we are actually in a transaction at the time, but because the user had not successfully logged in, it should not have been considered an active session.

          The attached change actually checks the SQLState for login errors and does not consider that case an active session. I wonder if there is not a better and more efficient way to check.

          Show
          Kathey Marsden added a comment - Attached is a patch which fixes this issue, but I don't know if it is the best solution. The critieria for a crash needs to be that we do the thread dump if we get a session level error while the session is active. To test whether the session was active we would just check database.isActive(). In the problematic authentication cases, database.isActive() was true and we are actually in a transaction at the time, but because the user had not successfully logged in, it should not have been considered an active session. The attached change actually checks the SQLState for login errors and does not consider that case an active session. I wonder if there is not a better and more efficient way to check.
          Hide
          Kathey Marsden added a comment -

          Marking patch available as I would like feedback on the solution. I ran AuthenticationTest but not the full set of regressions yet.

          Show
          Kathey Marsden added a comment - Marking patch available as I would like feedback on the solution. I ran AuthenticationTest but not the full set of regressions yet.
          Hide
          Kathey Marsden added a comment -

          oops, forgot to attach the test diff.

          Show
          Kathey Marsden added a comment - oops, forgot to attach the test diff.
          Hide
          Lily Wei added a comment -

          +1 The change looks good to me.

          Show
          Lily Wei added a comment - +1 The change looks good to me.
          Hide
          Kathey Marsden added a comment -

          Thanks Lily for the review. Regression's passed fine. I'll go ahead and check this into trunk and 10.8.

          Show
          Kathey Marsden added a comment - Thanks Lily for the review. Regression's passed fine. I'll go ahead and check this into trunk and 10.8.
          Hide
          Kathey Marsden added a comment -

          This issue affects 10.7 as well, but I am encountering conflicts backporting the fix. I think I won't bother with that now.
          If someone using 10.7 needs it they can look at the back port.

          Show
          Kathey Marsden added a comment - This issue affects 10.7 as well, but I am encountering conflicts backporting the fix. I think I won't bother with that now. If someone using 10.7 needs it they can look at the back port.
          Hide
          Knut Anders Hatlen added a comment -

          [bulk update: close all resolved issues that haven't had any activity the last year]

          Show
          Knut Anders Hatlen added a comment - [bulk update: close all resolved issues that haven't had any activity the last year]

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development