Derby
  1. Derby
  2. DERBY-210

Network Server will leak prepared statements if not explicitly closed by the user until the connection is closed

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 10.0.2.0
    • Fix Version/s: None
    • Component/s: Network Client
    • Urgency:
      Normal

      Description

      Network server will not garbage collect prepared statements that are not explicitly closed by the user. So a loop like this will leak.

      ...
      PreparedStatement ps;

      for (int i = 0 ; i < numPs; i++)
      {

      ps = conn.prepareStatement(selTabSql);
      rs =ps.executeQuery();

      while (rs.next())

      { rs.getString(1); }

      rs.close();
      // I'm a sloppy java programmer
      //ps.close();

      }

      To reproduce run the attached program
      java derbyStress

      Both client and server will grow until the connection is closed.

      It is likely that the fix for this will have to be in the client. The client does not send protocol to close the prepared statement, but rather reuses the PKGNAMCSN on the PRPSQLSTT request once the prepared statement has been closed. This is how the server knows to close the old statement and create a new one.

      1. derby-210-patch1.diff
        2 kB
        Deepa Remesh
      2. derby-210-patch2.diff
        8 kB
        Deepa Remesh
      3. derby-210-patch2.status
        0.6 kB
        Deepa Remesh
      4. derby-210-patch3.diff
        0.8 kB
        Deepa Remesh
      5. derby-210-patch4-v2.diff
        6 kB
        Deepa Remesh
      6. derby-210-patch4-v2.status
        0.1 kB
        Deepa Remesh
      7. derby-210-patch4-v3.diff
        7 kB
        Deepa Remesh
      8. derby-210-patch4-v3.status
        0.1 kB
        Deepa Remesh
      9. derby-210-patch5-v1.diff
        10 kB
        Deepa Remesh
      10. derby-210-patch5-v1.status
        0.4 kB
        Deepa Remesh
      11. derby-210-v2-draft.diff
        19 kB
        Deepa Remesh
      12. derby-210-v2-draft.status
        0.8 kB
        Deepa Remesh
      13. derbyStress.java
        12 kB
        Kathey Marsden
      14. DOTS_ATCJ2_Derby-noPatch.png
        6 kB
        John H. Embretsen
      15. DOTS_ATCJ2_Derby-withPatch.png
        7 kB
        John H. Embretsen
      16. runtimeinfo_DOTS-OOME.txt
        32 kB
        John H. Embretsen
      17. StatementStress.java
        17 kB
        John H. Embretsen

        Issue Links

          Activity

          Kathey Marsden created issue -
          Kathey Marsden made changes -
          Field Original Value New Value
          Attachment derbyStress.java [ 19583 ]
          Kathey Marsden made changes -
          Component/s Network Server [ 11410 ]
          Component/s Network Client [ 11690 ]
          Deepa Remesh made changes -
          Assignee Deepa Remesh [ deepa ]
          Deepa Remesh made changes -
          Comment [ To recap the problem:
          If PreparedStatement objects are not explicitly closed by user, they do not get garbage collected. They remain alive in the client driver even after the user application does not have any reference to them. The corresponding objects in network server are also not garbage collected.

          On looking at the client code, in addition to the reference returned to the user application, PreparedStatement objects are referenced at following places:
          1. In org.apache.derby.client.am.Connection, they are added to a java.util.LinkedList 'openStatements_'
          2. Another reference is stored in NetStatement and NetPreparedStatement.

          Thanks to Bernt and Kathey for suggestion to use WeakReference. I changed these to use WeakReferences and then I can see the PreparedStatement objects are getting finalized and collected both at client and server side. I could run >32K prepared statements without calling close. Before this change, I could run only ~950 statements before I get OutOfMemory error.

          For #1, using WeakReference seems to be the solution to me. But for #2, I was thinking if there is any other way we can avoid having cross references between Statement and NetStatement. One thing which I am not clear about is the structure of Statement classes. I see that NetConnection and NetResultSet extend Connection and ResultSet classes in client.am. But in case of Statement, the Net*Statement classes do not extend the corresponding Statement classes. They implement MaterialStatement interface. When constructing new Statement objects, a NetStatement object is created which internally creates and returns a Statement object. A reference to Statement object is added to the NetStatement object and reference to NetStatement (MaterialStatement) is added to the Statement object. Only when a Statement close() is called, the corresponding MaterialStatement is closed and its reference to Statement object gets removed.

          Since MaterialStatement is an interface, can Net*Statement classes implement this interface and also extend the Statement class? Is there any known reason for not doing this? I am working on changes for this. I just wanted to know if I should pursue this or stick to the original solution which seems to work. I would appreciate any suggestions for this. Thanks. ]
          Deepa Remesh made changes -
          Attachment derby-210-patch1.diff [ 12321949 ]
          Attachment derby-210-patch1.status [ 12321950 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch1.diff [ 12321949 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch1.status [ 12321950 ]
          Deepa Remesh made changes -
          Attachment derby-210.diff [ 12321980 ]
          Attachment derby-210.status [ 12321981 ]
          Deepa Remesh made changes -
          Attachment derby-210_10.1.diff [ 12322025 ]
          Attachment derby-210_10.1.status [ 12322026 ]
          Deepa Remesh made changes -
          Attachment derby-210_10.1.diff [ 12322025 ]
          Deepa Remesh made changes -
          Attachment derby-210_10.1.status [ 12322026 ]
          John H. Embretsen made changes -
          Attachment DOTS_ATCJ2_Derby-noPatch.png [ 12322534 ]
          Attachment DOTS_ATCJ2_Derby-withPatch.png [ 12322535 ]
          Deepa Remesh made changes -
          Attachment derby-210-v2-draft.diff [ 12322806 ]
          Attachment derby-210-v2-draft.status [ 12322807 ]
          Deepa Remesh made changes -
          Attachment derby-210.diff [ 12321980 ]
          Deepa Remesh made changes -
          Attachment derby-210.status [ 12321981 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch1.diff [ 12322844 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch2.status [ 12322862 ]
          Attachment derby-210-patch2.diff [ 12322861 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch3.diff [ 12322931 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch4.status [ 12322933 ]
          Attachment derby-210-patch4.diff [ 12322932 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch4.diff [ 12322932 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch4.status [ 12322933 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch4-v2.status [ 12322970 ]
          Attachment derby-210-patch4-v2.diff [ 12322969 ]
          Deepa Remesh made changes -
          Other Info [Patch available]
          Description Network server will not garbage collect prepared statements that are not explicitly closed by the user. So a loop like this will leak.

          ...
          PreparedStatement ps;

           for (int i = 0 ; i < numPs; i++)
          {

          ps = conn.prepareStatement(selTabSql);
          rs =ps.executeQuery();

          while (rs.next())
          {
          rs.getString(1);
          }
          rs.close();
          // I'm a sloppy java programmer
          //ps.close();

          }

          To reproduce run the attached program
          java derbyStress

          Both client and server will grow until the connection is closed.

           
          It is likely that the fix for this will have to be in the client. The client does not send protocol to close the prepared statement, but rather reuses the PKGNAMCSN on the PRPSQLSTT request once the prepared statement has been closed. This is how the server knows to close the old statement and create a new one.
          Network server will not garbage collect prepared statements that are not explicitly closed by the user. So a loop like this will leak.

          ...
          PreparedStatement ps;

           for (int i = 0 ; i < numPs; i++)
          {

          ps = conn.prepareStatement(selTabSql);
          rs =ps.executeQuery();

          while (rs.next())
          {
          rs.getString(1);
          }
          rs.close();
          // I'm a sloppy java programmer
          //ps.close();

          }

          To reproduce run the attached program
          java derbyStress

          Both client and server will grow until the connection is closed.

           
          It is likely that the fix for this will have to be in the client. The client does not send protocol to close the prepared statement, but rather reuses the PKGNAMCSN on the PRPSQLSTT request once the prepared statement has been closed. This is how the server knows to close the old statement and create a new one.
          Environment
          Kathey Marsden made changes -
          Comment [ Thank you Deepa for the explanations. I think I finally have my head around this change.
           I think there actually is a problem with not sending CLSQRY for finalize. If I have 20,000 open valid and referenced prepared statements and then I dereference them, I will continue to have 20,000 open statements on the server until the connection ends. Those statements will get reused if I prepare more statements, but will never actually get cleaned up until the connection ends.

          The reason for the change was to take the autocommit out of finalize and with that I fully agree, but I wonder: why do we have to take out the CLSQRY? writeCloseResultSets takes a boolean allowAutocommits. Could we continue to call it for finalize but just with that parameter set to false?

          Also I would appreciate input from others on this. Sorry Deepa to string this along so long.
          ]
          Deepa Remesh made changes -
          Other Info [Patch available]
          Deepa Remesh made changes -
          Link This issue incorporates DERBY-1021 [ DERBY-1021 ]
          Deepa Remesh made changes -
          Link This issue is blocked by DERBY-1002 [ DERBY-1002 ]
          Deepa Remesh made changes -
          Attachment derby-210-patch4-v3.status [ 12323383 ]
          Attachment derby-210-patch4-v3.diff [ 12323382 ]
          Deepa Remesh made changes -
          Other Info [Patch available]
          Deepa Remesh made changes -
          Attachment derby-210-patch5-v1.diff [ 12323602 ]
          Attachment derby-210-patch5-v1.status [ 12323603 ]
          John H. Embretsen made changes -
          Attachment StatementStress.java [ 12323684 ]
          John H. Embretsen made changes -
          Attachment runtimeinfo_DOTS-OOME.txt [ 12323920 ]
          Deepa Remesh made changes -
          Assignee Deepa Remesh [ deepa ]
          Andrew McIntyre made changes -
          Other Info [Patch available]
          Derby Info [Patch Available]
          Deepa Remesh made changes -
          Derby Info [Patch Available]
          Deepa Remesh made changes -
          Link This issue is blocked by DERBY-1002 [ DERBY-1002 ]
          Kathey Marsden made changes -
          Affects Version/s 10.0.2.0 [ 10920 ]
          Mike Matrigali made changes -
          Urgency Normal
          Kathey Marsden made changes -
          Labels derby_triage10_5_2
          Kathey Marsden made changes -
          Labels derby_triage10_5_2 derby_triage10_9
          Gavin made changes -
          Workflow jira [ 41590 ] Default workflow, editable Closed status [ 12796910 ]

            People

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

              Dates

              • Created:
                Updated:

                Development