Commons Pool
  1. Commons Pool
  2. POOL-189

close() does not release threads blocked on borrowObject()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.6
    • Fix Version/s: 1.5.7
    • Labels:
      None
    • Environment:

      java 1.6.0_24

      Description

      Hi,

      When I call close() and borrowObject() concurrently with no objects in
      the pool, close() will finish but borrowObject() is blocked forever.
      The documentation of close() mentions that borrowObject() should fail
      with IllegalStateException, so I would expect the waiting threads to
      throw IllegalStateException.

      I will attach a test that exposes this problem. For this test, the
      expected output is:
      0
      1
      2
      ...
      4999
      DONE

      But when the bug manifests (almost always for this test), the output
      is:
      0
      1
      2

      The test does not finish, it just gets stuck after printing several
      values.

      Is this a bug or am I reading the documentation wrongly? Is there
      a patch for this?

      Thanks!

      Adrian

      1. BUG-189v1.diff
        3 kB
        William R. Speirs
      2. BUG-189v2.diff
        6 kB
        William R. Speirs
      3. Test.java
        2 kB
        Adrian Nistor

        Activity

        Hide
        Phil Steitz added a comment -

        Thanks for reporting this.

        Show
        Phil Steitz added a comment - Thanks for reporting this.
        Hide
        William R. Speirs added a comment -

        Created a patch which I think fixes this issue. When the close() method is called it goes through the _allocationQueue and notifies all of the waiting threads. Then in the barrowObject() method, there is a check to see if the pool is closed after a waiting thread wakes. If so, the thread throws an IllegalStateException with a message of "Pool closed".

        I also included a unit test which tests this bug.

        Note: I was working off /commons/proper/pool/branches/POOL_1_X as I couldn't find a branch explicitly for 1.5.x code, and I figured trunk was 2.x code.

        Show
        William R. Speirs added a comment - Created a patch which I think fixes this issue. When the close() method is called it goes through the _allocationQueue and notifies all of the waiting threads. Then in the barrowObject() method, there is a check to see if the pool is closed after a waiting thread wakes. If so, the thread throws an IllegalStateException with a message of "Pool closed". I also included a unit test which tests this bug. Note: I was working off /commons/proper/pool/branches/POOL_1_X as I couldn't find a branch explicitly for 1.5.x code, and I figured trunk was 2.x code.
        Hide
        William R. Speirs added a comment -

        If the above patch is correct/accepted, then the same logic can easily be applied to GenericKeyedObjectPool as well.

        Show
        William R. Speirs added a comment - If the above patch is correct/accepted, then the same logic can easily be applied to GenericKeyedObjectPool as well.
        Hide
        Phil Steitz added a comment -

        Thanks, Bill! You used the right branch and my initial reaction is this looks good. You are right that this also affects GKOP and a similar fix should work there. I will look more carefully at this tonight. Additional review / comments welcome. Thanks for the patch!

        Show
        Phil Steitz added a comment - Thanks, Bill! You used the right branch and my initial reaction is this looks good. You are right that this also affects GKOP and a similar fix should work there. I will look more carefully at this tonight. Additional review / comments welcome. Thanks for the patch!
        Hide
        William R. Speirs added a comment -

        Included changes to GenericKeyedObjectPool and associated test.

        Show
        William R. Speirs added a comment - Included changes to GenericKeyedObjectPool and associated test.
        Hide
        Phil Steitz added a comment -

        I am sorry, but I am still not finished performance testing the patch. One round of tests made it look like I could measure the difference in performance du to the added check on each wake/borrow sequence. The difference is not statistically significant, but appears measurable. I am running some longer running tests to see if this is a real effect.

        Show
        Phil Steitz added a comment - I am sorry, but I am still not finished performance testing the patch. One round of tests made it look like I could measure the difference in performance du to the added check on each wake/borrow sequence. The difference is not statistically significant, but appears measurable. I am running some longer running tests to see if this is a real effect.
        Hide
        William R. Speirs added a comment -

        Wondering how the performance tests are coming along?

        A thought. I wouldn't be surprised if the if(...) check does take a bit longer, but does it really matter? The thread has already been asleep waiting on the pool to grow. I would say argue that performing the check to see if the pool has been closed is simply "part of growing the pool". Thoughts?

        Show
        William R. Speirs added a comment - Wondering how the performance tests are coming along? A thought. I wouldn't be surprised if the if(...) check does take a bit longer, but does it really matter? The thread has already been asleep waiting on the pool to grow. I would say argue that performing the check to see if the pool has been closed is simply "part of growing the pool". Thoughts?
        Hide
        Phil Steitz added a comment -

        I could not measure any impact to performance. Second patch committed in r1205211. Thanks!

        Show
        Phil Steitz added a comment - I could not measure any impact to performance. Second patch committed in r1205211. Thanks!

          People

          • Assignee:
            Unassigned
            Reporter:
            Adrian Nistor
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development