Derby
  1. Derby
  2. DERBY-5683

BaseJDBCTestCase.getDatabaseProperty() should close resources before returning

    Details

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

      Description

      BaseJDBCTestCase.getDatabaseProperty() creates a PreparedStatement and a ResultSet that are not closed before the method returns. If a test case calls getDatabaseProperty() many times, the open statements may have a big memory footprint.

      This can be seen for example by running jdbcapi.AuthenticationTest with -Xmx64. It will fail with an OutOfMemoryError when testNoCollisionsWithConfigurableHash() runs in client/server mode.

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        Attaching a patch that makes getDatabaseProperty() close the ResultSet and PreparedStatement before returning the property value. It also removes the PreparedStatement from the list of statements to close at tearDown(). With these changes, AuthenticationTest completes successfully in my environment when running with -Xmx64.

        Show
        Knut Anders Hatlen added a comment - Attaching a patch that makes getDatabaseProperty() close the ResultSet and PreparedStatement before returning the property value. It also removes the PreparedStatement from the list of statements to close at tearDown(). With these changes, AuthenticationTest completes successfully in my environment when running with -Xmx64.
        Hide
        Dag H. Wanvik added a comment -

        Looks like an improvement. +1 Nit: the new name closeStatement doesn't convey the semantics for "forgetting/removing" the statement from "statements". What about "removeStatement"?

        Show
        Dag H. Wanvik added a comment - Looks like an improvement. +1 Nit: the new name closeStatement doesn't convey the semantics for "forgetting/removing" the statement from "statements". What about "removeStatement"?
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for reviewing the patch, Dag.

        I think I prefer to keep the name "closeStatement", as the logical operation of the method is to close the statement. Removing it from the "statements" list is just internal bookkeeping. Also, the "closeStatement" method is the inverse of the "createStatement", "prepareStatement" and "prepareCall" methods in BaseJDBCTestCase, which don't have the "remembering/adding" aspect reflected in their names.

        Committed revision 1310413.

        Show
        Knut Anders Hatlen added a comment - Thanks for reviewing the patch, Dag. I think I prefer to keep the name "closeStatement", as the logical operation of the method is to close the statement. Removing it from the "statements" list is just internal bookkeeping. Also, the "closeStatement" method is the inverse of the "createStatement", "prepareStatement" and "prepareCall" methods in BaseJDBCTestCase, which don't have the "remembering/adding" aspect reflected in their names. Committed revision 1310413.

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development