Bug 52066 - ConnectionPool.borrowConnection swallows interrupt state.
Summary: ConnectionPool.borrowConnection swallows interrupt state.
Status: RESOLVED FIXED
Alias: None
Product: Tomcat Modules
Classification: Unclassified
Component: jdbc-pool (show other bugs)
Version: unspecified
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-20 19:28 UTC by Alexander Pogrebnyak
Modified: 2012-03-29 16:54 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pogrebnyak 2011-10-20 19:28:24 UTC
In this code snippet starting on line 6.15 of ConnectionPool.java version 7.0.22

    try {
        //retrieve an existing connection
        con = idle.poll(timetowait, TimeUnit.MILLISECONDS);
    } catch (InterruptedException ex) {
        Thread.interrupted();//clear the flag, and bail out
        SQLException sx = new SQLException("Pool wait interrupted.");
        sx.initCause(ex);
        throw sx;
    } finally {
        waitcount.decrementAndGet();
    }

The line marked '//clear the flag, and bail out' is wrong, because after that there is no way for calling code to find out that the thread has been interrupted.

The correct behavior should be

    Thread.currentThread( ).interrupt( );

Here is the excerpt from the Biran Goetz's "Java Concurrency in Practice" chapter 5.4.

<!START QUOTATION>
For library code there are basically two choices:

-- Propagate the InterruptedException. This is often the most sensible policy if you can get away with it -- just propagate the InterruptedException to your caller.  This could involve not catching InterruptedException, or catching it and throwing it again after performing some brief activity-specific cleanup. 

-- Restore the interrupt. Sometimes you cannot throw InterruptedException, for instance when your code is part of a Runnable. In these situations, you must catch InterruptedException and restore the interrupted status by calling interrupt on the current thread, so that code higher up the call stack can see that an interrupt was issued.
<!END QUOTATION>

In the case of borrowConnection, #1 is not really a choice, as it is running in the confines of JDBC interface.  But it can definitely restore the interrupt status of the thread.
Comment 1 Konstantin Kolinko 2011-10-26 14:13:51 UTC
1. Thread.interrupted() - clears the flag
2. new SQLException() - bails out

It does sx.initCause(ex), so original InterruptedException is still there.

Do you have trouble dealing with this SQLException?

Whether you should follow the advise from the book depends on why the interruption was requested. I do not see any problem here.
Comment 2 Alexander Pogrebnyak 2011-10-26 14:31:26 UTC
(In reply to comment #1)

There may be quite a few clients down the call chain that require the knowledge that interrupt is in progress, and clearing of the interrupted flag destroys this knowledge.

It is semantically very similar to the olden days practice of swallowing exception in the library code, e.g.

try
{
   doSomethingRisky( );
}
catch ( Throwable ex )
{
}

Plus, it's not OK for the general user of the library to depend on your specific wrapper of InterruptedException into the SQLException.  A user can switch to another implementation of connection pool and that particular library reporting policy may not be the same as this library's. 

The right thing to do is to preserve the thread interruption status by calling Thread.currentThread( ).interrupt( ), and let the client code deal with it.

BTW, setting InterruptedException as a cause of SQLException is a GOOD THING, I don't want you to change that.
Comment 3 Filip Hanik 2012-03-20 16:32:55 UTC
(In reply to comment #2)
> (In reply to comment #1)
> 

> 
> Plus, it's not OK for the general user of the library to depend on your
> specific wrapper of InterruptedException into the SQLException.  A user can
> switch to another implementation of connection pool and that particular library
> reporting policy may not be the same as this library's. 

I am not sure I agree with this. The user has not invoked an "interruptable" action, the user invoked DataSource.getConnection
http://docs.oracle.com/javase/6/docs/api/javax/sql/DataSource.html#getConnection()
This call says nothing about interruption nor does it declare it.

It is a totally different call than poll(...)
http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/BlockingQueue.html#poll%28long,%20java.util.concurrent.TimeUnit%29

> 
> The right thing to do is to preserve the thread interruption status by calling
> Thread.currentThread( ).interrupt( ), and let the client code deal with it.

The reason an interrupt here would happen is cause we want to interrupt the call to queue.poll, to the user, they need to be notified that their call will fail using a SQLException. 

> 
> BTW, setting InterruptedException as a cause of SQLException is a GOOD THING, I
> don't want you to change that.

We do specify the root cause for troubleshooting reasons. If we don't clear the flag, the thread can continue to run with a interrupted status, and I'm not sure that is a good thing.

Feel free to reopen with a use case that may change the course of this resolution.
Comment 4 Alexander Pogrebnyak 2012-03-20 17:34:44 UTC
(In reply to comment #3)
 
> The reason an interrupt here would happen is cause WE want to interrupt the
> call to queue.poll, to the user, they need to be notified that their call will
> fail using a SQLException. 
> 

I capitalized WE in the above quote to highlight the wrong approach to this problem.

YOU, the tomcat-jdbc, would never interrupt a thread that is stuck in a `borrowConnection`.  This action can only be done by the user of your library.

> If we don't clear the
> flag, the thread can continue to run with a interrupted status, and I'm not
> sure that is a good thing.

it IS a good thing, because, once again, your library should never initiate the interruption, and if the user does not handle interruption correctly it will get reminded sooner rather than later that something is wrong.  Think of it as a RuntimeException.  If you know how to hanlde it you will, if you don't let the framework handle it.

Swallowing interrupted status is exactly the same issue that plagued Java samples in the early days, when Exception would be swallowed and nothing reported to the caller.
Comment 5 Filip Hanik 2012-03-27 15:16:28 UTC
Our library could definitely initiate the interruption. The easiest use case would be pool.close() to get rid of all the waiting threads. and the waiting thread in this case, would receive a SQLException

In the above use case, it should absolutely be cleared.

Since the waiting thread receives the SQLException, I am still not convinced. the call DataSource.getConnection does not have any interrupted exceptions associated with it.
Comment 6 Alexander Pogrebnyak 2012-03-27 17:03:49 UTC
(In reply to comment #5)
> Our library could definitely initiate the interruption. The easiest use case
> would be pool.close() to get rid of all the waiting threads. and the waiting
> thread in this case, would receive a SQLException
> 
> In the above use case, it should absolutely be cleared.
> 
> Since the waiting thread receives the SQLException, I am still not convinced.
> the call DataSource.getConnection does not have any interrupted exceptions
> associated with it.

OK, Let's get to this problem from the other end.  Is it possible to provide a user preference that by default executes a current code, but when set preserved the thread interrupted status?

I really hate to implement yet another facade to your otherwise feature complete product.

If this approach is acceptable to you I am willing to contribute the required patch for review.
Comment 7 Filip Hanik 2012-03-27 17:59:03 UTC
Good idea 

Fixed in
 r1305931 
 r1305933
Comment 8 Filip Hanik 2012-03-28 15:25:49 UTC
Please note that the interrupt flag was already cleared, this commit does change things around a bit
http://svn.apache.org/viewvc?rev=1306410&view=rev
I've made an adjustment based on a suggestion on the dev list
Comment 9 Alexander Pogrebnyak 2012-03-29 14:52:47 UTC
(In reply to comment #8)
> Please note that the interrupt flag was already cleared, this commit does
> change things around a bit
> http://svn.apache.org/viewvc?rev=1306410&view=rev
> I've made an adjustment based on a suggestion on the dev list

Reviewing the code I see these lines in close() and borrowConnection

    catch (InterruptedException ex) {
        ...
        if (!getPoolProperties().getPropagateInterruptState()) {
           Thread.interrupted();
        }
        ...
    }

The problem is that this still does not set the interrupted status on the thread, that's the common behavior of many flavors of wait methods.  So, in this case the handler have to make a call to interrupt itself.

The exception handler code should do this:

    catch (InterruptedException ex) {
        ...
        if (getPoolProperties().getPropagateInterruptState()) {
           Thread.currentThread().interrupt();
        } else {
           Thread.interrupted();
        }
        ...
    }

The fix should be applied to lines 384 and and 631 of org.apache.tomcat.jdbc.pool.ConnectionPool in http://svn.apache.org/viewvc?view=revision&revision=1305931
Comment 10 Alexander Pogrebnyak 2012-03-29 14:57:47 UTC
BTW, it is not related to this bug, but I don't know where to submit it otherwise.

When I add a comment to this bug report and press `Save Changes` button Bugzilla does not redirect we to this report (52066), but instead goes to "ASF Bugzilla – Bug 48001".

48001 is completely unrelated report, but it was also submitted by me.

If you could kindly reply to this comment with a proper channel I will submit this bug there.  Thanks!
Comment 11 Filip Hanik 2012-03-29 16:19:53 UTC
http://www.apache.org/dev/#infrastructure
Comment 12 Alexander Pogrebnyak 2012-03-29 16:30:01 UTC
(In reply to comment #11)
> http://www.apache.org/dev/#infrastructure

Created bug report https://issues.apache.org/jira/browse/INFRA-4620
Comment 13 Alexander Pogrebnyak 2012-03-29 16:32:21 UTC
Reopening this bug, as there is no reply to my comment #9 -> https://issues.apache.org/bugzilla/show_bug.cgi?id=52066#c9.
Comment 14 Filip Hanik 2012-03-29 16:37:34 UTC
r1306410

a day or so ago
Comment 15 Alexander Pogrebnyak 2012-03-29 16:52:27 UTC
(In reply to comment #14)
> r1306410
> 
> a day or so ago

Thanks a lot for fixing this!!!
Comment 16 Alexander Pogrebnyak 2012-03-29 16:54:55 UTC
Dummy comment to test https://issues.apache.org/jira/browse/INFRA-4620

Please disregard.