Commons Pool
  1. Commons Pool
  2. POOL-179

GenericObjectPool.borrowObject() incorrectly swallows InterruptedException

    Details

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

      mac osx 10.5.8 jdk 1.6.0_22

      Description

      I just updated from commpons-pool 1.5.4 to 1.5.5 and suddenly some of my tests crash with threads hanging.

      After some inspection is appears that GenericObjectPool.borrowObject() no longer handles InterruptedException correctly. I made a file diff and found that there has been a modification in that area that contains the bug. See attached image.

      I have created a patched version absed on 1.5.4 source which works correctly in my use case. The important part is:

      catch (final InterruptedException e)
      {
         synchronized (this)
         {
            if (latch.getPair() == null)
            {
               _allocationQueue.remove(latch);
            }
            else
            {
               _numInternalProcessing--;
               _numActive++;
               _allocationQueue.remove(latch);
               returnObject(latch.getPair().value);
            }
         }
         Thread.currentThread().interrupt();
         throw e;
      }
      
      1. FileDiff154-155.jpg
        72 kB
        Axel Großmann

        Activity

        Hide
        Phil Steitz added a comment -

        Thanks for reporting this. I think what is going on is that threads are being interrupted after they have been allocated objects by the pool. See the comments in POOL-162. Perhaps we should change the logic to "undo" allocation in this case.

        Show
        Phil Steitz added a comment - Thanks for reporting this. I think what is going on is that threads are being interrupted after they have been allocated objects by the pool. See the comments in POOL-162 . Perhaps we should change the logic to "undo" allocation in this case.
        Hide
        Axel Großmann added a comment -

        I think the important bit is that a InterruptedException must never be swallowed but passed on to the calling code. And when the pool is doing so of course any allocated element must be put back into the pool.

        The code that did not work with 1.5.5 looks somewhat like this:

        try
        {
           while( !Thread.currentThread.isInterrupted())
           {
              element = pool.borrowObject();
              ...
            }
        }
        catch( InterruptedException e)
        {
           // ok, someone wants me to stop
        }
        

        If borrowObject() would swallow the exception my thread would never notice that it has been interrupted and therefore would wrongly continue to work forever. Note that the sample is probably overdoing things a bit since it catches InterruptedException and checks whether the current thread is being interrupted, but the general idea should be clear.

        Show
        Axel Großmann added a comment - I think the important bit is that a InterruptedException must never be swallowed but passed on to the calling code. And when the pool is doing so of course any allocated element must be put back into the pool. The code that did not work with 1.5.5 looks somewhat like this: try { while ( ! Thread .currentThread.isInterrupted()) { element = pool.borrowObject(); ... } } catch ( InterruptedException e) { // ok, someone wants me to stop } If borrowObject() would swallow the exception my thread would never notice that it has been interrupted and therefore would wrongly continue to work forever. Note that the sample is probably overdoing things a bit since it catches InterruptedException and checks whether the current thread is being interrupted, but the general idea should be clear.
        Hide
        Phil Steitz added a comment -

        The patch in the first comment looks correct to me. Unless others disagree, I will commit it.

        Show
        Phil Steitz added a comment - The patch in the first comment looks correct to me. Unless others disagree, I will commit it.
        Hide
        Simone Tripodi added a comment -

        no objections from my side. does this issue affects the 2.0 too?

        Show
        Simone Tripodi added a comment - no objections from my side. does this issue affects the 2.0 too?
        Hide
        Phil Steitz added a comment -

        The current code in the 2.0 branch contains this same bug. I will port the fix to the 2.0 code for the time being (we have talked about replacing the core implementation in 2.0).

        Show
        Phil Steitz added a comment - The current code in the 2.0 branch contains this same bug. I will port the fix to the 2.0 code for the time being (we have talked about replacing the core implementation in 2.0).
        Hide
        Mark Thomas added a comment -

        I think there are three cases we need to handle:
        a) latch has been allocated an object
        b) latch has been given permission to create an object
        c) latch is still in queue

        I think there are a couple of issues with the patch above:

        • it tries to remove the latch from the queue in cases a) & b) but the latch will already have been removed (should just be a no-op but still makes sense not to make the call)
        • it doesn't handle case c)

        I have a patch in mind based on the patch above but I'd really like a test case for this but cases a) & b) look like they are going to be tricky to test.

        There is also the fact that this code is rather tricky and it has been a while since I last looked at it. I may be completely wrong in my analysis so an extra pair of eyes would be good here.

        Show
        Mark Thomas added a comment - I think there are three cases we need to handle: a) latch has been allocated an object b) latch has been given permission to create an object c) latch is still in queue I think there are a couple of issues with the patch above: it tries to remove the latch from the queue in cases a) & b) but the latch will already have been removed (should just be a no-op but still makes sense not to make the call) it doesn't handle case c) I have a patch in mind based on the patch above but I'd really like a test case for this but cases a) & b) look like they are going to be tricky to test. There is also the fact that this code is rather tricky and it has been a while since I last looked at it. I may be completely wrong in my analysis so an extra pair of eyes would be good here.
        Hide
        Phil Steitz added a comment -

        I agree with you on the three cases, Mark and in a) and b) the latch will have already been removed. I think the first part of the patch tries to handle c), but is not quite right now that I think more about it because the if test will also succeed if the latch has been removed and mayCreate. I think the patch tries to do the right thing if the latch has been served. I will work on a test case and review whatever you come up with. Seems to me we need to do the following in your cases
        a) return the object to the queue and decrement the internal processing count
        b) decrement internal processing count
        c) remove the latch from the queue

        Show
        Phil Steitz added a comment - I agree with you on the three cases, Mark and in a) and b) the latch will have already been removed. I think the first part of the patch tries to handle c), but is not quite right now that I think more about it because the if test will also succeed if the latch has been removed and mayCreate. I think the patch tries to do the right thing if the latch has been served. I will work on a test case and review whatever you come up with. Seems to me we need to do the following in your cases a) return the object to the queue and decrement the internal processing count b) decrement internal processing count c) remove the latch from the queue
        Hide
        Mark Thomas added a comment -

        Fixed for 1.5.6 and the current trunk.

        Show
        Mark Thomas added a comment - Fixed for 1.5.6 and the current trunk.

          People

          • Assignee:
            Mark Thomas
            Reporter:
            Axel Großmann
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development