Thread: BUG org.postgresql.Driver.connect() distorts InterruptedException

Hi there!

+ JDBC driver build number
9.1-903

+ Server
PostgreSQL 9.1.9 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian 4.4.5-8) 4.4.5, 32-bit

+ Error message / stack trace

org.postgresql.util.PSQLException: Beim Verbindungsversuch trat eine Unterbrechung auf.
 at org.postgresql.Driver$ConnectThread.getResult(Driver.java:365)
 at org.postgresql.Driver.connect(Driver.java:269)
…

+ Description

Recently I discovered this problem because it prevents to tell other SQLExceptions and interruption apart (in the
exceptionhandlers). After examining the source of org.postgresql.Driver I noticed that
org.postgresql.Driver$ConnectThread.getResultcatches InterruptedException and throws a
org.postgresql.util.PSQLExceptioninstead BUT without supplying the InterruptedException as the cause (exception
chaining)and without resetting the interrupted flag via Thread.currentThread().interrupt() (SQL error code / SQL state
isunspecific, too). Please note that this behavior is present in 9.2-1003, too. 

Therefore I suggest that the handling of InterruptedException should be changed so it becomes more interoperable with
clientcode that needs to handle SQLExceptions differently from thread interruption -- in a concurrent environment an
alreadyscheduled database query may become obsolete before it finishes because another query may be needed for example
becauseof user intervention. 

Greetings

Hi,

InterruptedException should be rewrapped only very carefully. When you
receive InterruptedException it means "this thread must die", so if
you have to rethrow something else due signature issues to you should
rethrow something that nobody will catch and swallow. I'd rethrow a
bare RuntimeException in need be.

Florent


On Mon, Aug 12, 2013 at 6:22 PM, Dave Cramer <pg@fastcrypt.com> wrote:
> Ok, so you would like the code to not reset the interrupted exception so
> that when it returns the interrupted bit will still be set and you can check
> for isInterrupted ?
>
> The SQL error is rather vague here since it is not really a SQL error,
> rather someone cancelled the thread.
>
>
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>
> On Fri, Aug 9, 2013 at 4:35 AM, Andreas Rudolph
> <andreas.rudolph@spontech-spine.com> wrote:
>>
>> Hi there!
>>
>> + JDBC driver build number
>> 9.1-903
>>
>> + Server
>> PostgreSQL 9.1.9 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian
>> 4.4.5-8) 4.4.5, 32-bit
>>
>> + Error message / stack trace
>>
>> org.postgresql.util.PSQLException: Beim Verbindungsversuch trat eine
>> Unterbrechung auf.
>>  at org.postgresql.Driver$ConnectThread.getResult(Driver.java:365)
>>  at org.postgresql.Driver.connect(Driver.java:269)
>> …
>>
>> + Description
>>
>> Recently I discovered this problem because it prevents to tell other
>> SQLExceptions and interruption apart (in the exception handlers). After
>> examining the source of org.postgresql.Driver I noticed that
>> org.postgresql.Driver$ConnectThread.getResult catches InterruptedException
>> and throws a org.postgresql.util.PSQLException instead BUT without supplying
>> the InterruptedException as the cause (exception chaining) and without
>> resetting the interrupted flag via Thread.currentThread().interrupt() (SQL
>> error code / SQL state is unspecific, too). Please note that this behavior
>> is present in 9.2-1003, too.
>>
>> Therefore I suggest that the handling of InterruptedException should be
>> changed so it becomes more interoperable with client code that needs to
>> handle SQLExceptions differently from thread interruption -- in a concurrent
>> environment an already scheduled database query may become obsolete before
>> it finishes because another query may be needed for example because of user
>> intervention.
>>
>> Greetings
>>
>> --
>> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-jdbc
>
>



--
Florent Guillaume, Director of R&D, Nuxeo
Open Source, Java EE based, Enterprise Content Management (ECM)
http://www.nuxeo.com   http://www.nuxeo.org   +33 1 40 33 79 87

Am 12.08.2013 um 18:22 schrieb Dave Cramer <pg@fastcrypt.com>:

> Ok, so you would like the code to not reset the interrupted exception so that when it returns the interrupted bit
willstill be set and you can check for isInterrupted ? 
It would be acceptable to set the interrupted flag again and throw a PSQLException without the cause set (to the
InterruptedException)to signal that no connection has been established to the caller. Then client code could catch the
exceptionand notice that the connect request failed. Additionally it could examine the interrupted flag. That way it
couldat least detected that it (the calling thread) has been interrupted. However, theoretically a PSQLException could
havebeen thrown for some other reason and at the same time interruption may occur. Because of that client code cannot
reliablytell whether the connection failed because of interruption. Client might want to know this is because it would
behavedifferently, for example it could decide to log connect failures except for interruption. 

>
> The SQL error is rather vague here since it is not really a SQL error, rather someone cancelled the thread.
I agree. Most of the time SQL errors refer to something that happened on the server. I just mentioned it for
completeness.

>
>
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>
> On Fri, Aug 9, 2013 at 4:35 AM, Andreas Rudolph <andreas.rudolph@spontech-spine.com> wrote:
> Hi there!
>
> + JDBC driver build number
> 9.1-903
>
> + Server
> PostgreSQL 9.1.9 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian 4.4.5-8) 4.4.5, 32-bit
>
> + Error message / stack trace
>
> org.postgresql.util.PSQLException: Beim Verbindungsversuch trat eine Unterbrechung auf.
>  at org.postgresql.Driver$ConnectThread.getResult(Driver.java:365)
>  at org.postgresql.Driver.connect(Driver.java:269)
> …
>
> + Description
>
> Recently I discovered this problem because it prevents to tell other SQLExceptions and interruption apart (in the
exceptionhandlers). After examining the source of org.postgresql.Driver I noticed that
org.postgresql.Driver$ConnectThread.getResultcatches InterruptedException and throws a
org.postgresql.util.PSQLExceptioninstead BUT without supplying the InterruptedException as the cause (exception
chaining)and without resetting the interrupted flag via Thread.currentThread().interrupt() (SQL error code / SQL state
isunspecific, too). Please note that this behavior is present in 9.2-1003, too. 
>
> Therefore I suggest that the handling of InterruptedException should be changed so it becomes more interoperable with
clientcode that needs to handle SQLExceptions differently from thread interruption -- in a concurrent environment an
alreadyscheduled database query may become obsolete before it finishes because another query may be needed for example
becauseof user intervention. 
>
> Greetings
>
> --
> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-jdbc
>

Am 13.08.2013 um 12:46 schrieb Florent Guillaume <fg@nuxeo.com>:

> Hi,
>
> InterruptedException should be rewrapped only very carefully. When you
> receive InterruptedException it means "this thread must die", so if
> you have to rethrow something else due signature issues to you should
> rethrow something that nobody will catch and swallow. I'd rethrow a
> bare RuntimeException in need be.
I think it's acceptable to wrap it in another checked exception if that exception signals that the request that has
beeninterrupted has failed because of interruption. Of course I agree that it would be better if a method that could
failin such a way would specify InterruptedException in its throws clause. But if you have to deal with frameworks that
aredesigned differently, for example JDBC, wrapping it as described seems to be a reasonable option. 

>
> Florent
>
>
> On Mon, Aug 12, 2013 at 6:22 PM, Dave Cramer <pg@fastcrypt.com> wrote:
>> Ok, so you would like the code to not reset the interrupted exception so
>> that when it returns the interrupted bit will still be set and you can check
>> for isInterrupted ?
>>
>> The SQL error is rather vague here since it is not really a SQL error,
>> rather someone cancelled the thread.
>>
>>
>>
>> Dave Cramer
>>
>> dave.cramer(at)credativ(dot)ca
>> http://www.credativ.ca
>>
>>
>> On Fri, Aug 9, 2013 at 4:35 AM, Andreas Rudolph
>> <andreas.rudolph@spontech-spine.com> wrote:
>>>
>>> Hi there!
>>>
>>> + JDBC driver build number
>>> 9.1-903
>>>
>>> + Server
>>> PostgreSQL 9.1.9 on i686-pc-linux-gnu, compiled by gcc-4.4.real (Debian
>>> 4.4.5-8) 4.4.5, 32-bit
>>>
>>> + Error message / stack trace
>>>
>>> org.postgresql.util.PSQLException: Beim Verbindungsversuch trat eine
>>> Unterbrechung auf.
>>> at org.postgresql.Driver$ConnectThread.getResult(Driver.java:365)
>>> at org.postgresql.Driver.connect(Driver.java:269)
>>> …
>>>
>>> + Description
>>>
>>> Recently I discovered this problem because it prevents to tell other
>>> SQLExceptions and interruption apart (in the exception handlers). After
>>> examining the source of org.postgresql.Driver I noticed that
>>> org.postgresql.Driver$ConnectThread.getResult catches InterruptedException
>>> and throws a org.postgresql.util.PSQLException instead BUT without supplying
>>> the InterruptedException as the cause (exception chaining) and without
>>> resetting the interrupted flag via Thread.currentThread().interrupt() (SQL
>>> error code / SQL state is unspecific, too). Please note that this behavior
>>> is present in 9.2-1003, too.
>>>
>>> Therefore I suggest that the handling of InterruptedException should be
>>> changed so it becomes more interoperable with client code that needs to
>>> handle SQLExceptions differently from thread interruption -- in a concurrent
>>> environment an already scheduled database query may become obsolete before
>>> it finishes because another query may be needed for example because of user
>>> intervention.
>>>
>>> Greetings
>>>
>>> --
>>> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-jdbc
>>
>>
>
>
>
> --
> Florent Guillaume, Director of R&D, Nuxeo
> Open Source, Java EE based, Enterprise Content Management (ECM)
> http://www.nuxeo.com   http://www.nuxeo.org   +33 1 40 33 79 87

On Tue, Aug 13, 2013 at 5:41 PM, Andreas Rudolph
<andreas.rudolph@spontech-spine.com> wrote:
> Am 13.08.2013 um 12:46 schrieb Florent Guillaume <fg@nuxeo.com>:
>> InterruptedException should be rewrapped only very carefully. When you
>> receive InterruptedException it means "this thread must die", so if
>> you have to rethrow something else due signature issues to you should
>> rethrow something that nobody will catch and swallow. I'd rethrow a
>> bare RuntimeException in need be.
> I think it's acceptable to wrap it in another checked exception if that exception signals that the request that has
beeninterrupted has failed because of interruption. Of course I agree that it would be better if a method that could
failin such a way would specify InterruptedException in its throws clause. But if you have to deal with frameworks that
aredesigned differently, for example JDBC, wrapping it as described seems to be a reasonable option. 

If you wrap it in a PSQLException then a JDBC connection pool could
take that as meaning that this connection has problems and should be
removed from the pool, but it won't check or propagate the interrupt,
and won't check the exception's cause either. Very few frameworks do.
That's why it's important that if you have to rewrap, you rewrap in an
exception that nobody risks catching without knowing that there's an
underlying interruption.

Florent

--
Florent Guillaume, Director of R&D, Nuxeo
Open Source, Java EE based, Enterprise Content Management (ECM)
http://www.nuxeo.com   http://www.nuxeo.org   +33 1 40 33 79 87

Am 13.08.2013 um 19:15 schrieb Florent Guillaume <fg@nuxeo.com>:

> On Tue, Aug 13, 2013 at 5:41 PM, Andreas Rudolph
> <andreas.rudolph@spontech-spine.com> wrote:
>> Am 13.08.2013 um 12:46 schrieb Florent Guillaume <fg@nuxeo.com>:
>>> InterruptedException should be rewrapped only very carefully. When you
>>> receive InterruptedException it means "this thread must die", so if
>>> you have to rethrow something else due signature issues to you should
>>> rethrow something that nobody will catch and swallow. I'd rethrow a
>>> bare RuntimeException in need be.
>> I think it's acceptable to wrap it in another checked exception if that exception signals that the request that has
beeninterrupted has failed because of interruption. Of course I agree that it would be better if a method that could
failin such a way would specify InterruptedException in its throws clause. But if you have to deal with frameworks that
aredesigned differently, for example JDBC, wrapping it as described seems to be a reasonable option. 
>
> If you wrap it in a PSQLException then a JDBC connection pool could
> take that as meaning that this connection has problems and should be
> removed from the pool, but it won't check or propagate the interrupt,
> and won't check the exception's cause either. Very few frameworks do.
> That's why it's important that if you have to rewrap, you rewrap in an
> exception that nobody risks catching without knowing that there's an
> underlying interruption.
In my bug report I was talking about another situation: The implementation of org.postgresql.Driver.connect(String url,
Propertiesinfo) does NOT leave any evidence that the calling thread (i.e. client code) has been interrupted while
waitingfor a Connection to be returned. In this situation there is no Connection at all, yet, that could be reused or
removedfrom a pool. 
(I guess instead a pool implementation would call Driver.connect() by itself to obtain a connection for the first time,
thatis the pool implementation would be a client of the Driver class.) 

Even if someone agrees with your statement that receiving an InterruptedException (always) means "this thread must die"
thisDOES NOT necessarily mean that it must die _immediately_! On the contrary it might need to do some clean-up,
releaseresources etc. Throwing an unchecked exception in the hope that it would not be caught is of no help in this
case.

The decision what an interrupt should mean to the thread in question, that is the thread that has been created by
clientcode and which executes Driver.connect(), is up to the programmer of the client code. Therefore I was asking to
improveinteroperability by allowing the client code to detect that the thread that executes it has been interrupted. 

Regarding exception handling with Driver.connect():
Clients must catch the _checked_ SQLException. If client code has caught this exception this is a clear indication that
no(usable) Connection object could have been returned otherwise. I can't see any evidence in your reasoning why
supplyingan InterruptedException as a cause of a checked exception would do any harm here. It just reveals the fact
thatthe connection could not be established because the requesting thread has been interrupted. 

Could you please tell why doing _both_, suppling a cause _and_ setting the interrupted flag again, in the case of
Driver.connect()seems unsound to you? 

Hi,

On Thu, Aug 15, 2013 at 9:07 AM, Andreas Rudolph
<andreas.rudolph@spontech-spine.com> wrote:
> Am 13.08.2013 um 19:15 schrieb Florent Guillaume <fg@nuxeo.com>:
>> If you wrap it in a PSQLException then a JDBC connection pool could
>> take that as meaning that this connection has problems and should be
>> removed from the pool, but it won't check or propagate the interrupt,
>> and won't check the exception's cause either. Very few frameworks do.
>> That's why it's important that if you have to rewrap, you rewrap in an
>> exception that nobody risks catching without knowing that there's an
>> underlying interruption.
>
> In my bug report I was talking about another situation: The implementation of org.postgresql.Driver.connect(String
url,Properties info) does NOT leave any evidence that the calling thread (i.e. client code) has been interrupted while
waitingfor a Connection to be returned. In this situation there is no Connection at all, yet, that could be reused or
removedfrom a pool. 
> (I guess instead a pool implementation would call Driver.connect() by itself to obtain a connection for the first
time,that is the pool implementation would be a client of the Driver class.) 

A pool was just an example of a library calling the driver and needing
to do some cleanup and retry when something goes wrong. In the connect
situation one could imagine code trying to re-obtain a connection
after a few seconds if it receives an exception on the first connect.

> Even if someone agrees with your statement that receiving an InterruptedException (always) means "this thread must
die"this DOES NOT necessarily mean that it must die _immediately_! On the contrary it might need to do some clean-up,
releaseresources etc. Throwing an unchecked exception in the hope that it would not be caught is of no help in this
case.

Yes cleanup of critical resources is legitimate and encouraged on interrupt.

> The decision what an interrupt should mean to the thread in question, that is the thread that has been created by
clientcode and which executes Driver.connect(), is up to the programmer of the client code. Therefore I was asking to
improveinteroperability by allowing the client code to detect that the thread that executes it has been interrupted. 

And I'm all for it. I think it's juste better to do that in a manner
that doesn't risk being mistaken with something else by client code
(which itself could be a library in not in the hands of the
application programmer himself).

> Regarding exception handling with Driver.connect():
> Clients must catch the _checked_ SQLException. If client code has caught this exception this is a clear indication
thatno (usable) Connection object could have been returned otherwise. I can't see any evidence in your reasoning why
supplyingan InterruptedException as a cause of a checked exception would do any harm here. It just reveals the fact
thatthe connection could not be established because the requesting thread has been interrupted. 
>
> Could you please tell why doing _both_, suppling a cause _and_ setting the interrupted flag again, in the case of
Driver.connect()seems unsound to you? 

Setting the interrupt flag is definitely a must do. Setting the cause
is also a nice addition but 99% of the code out there will not check
the cause to determine that nothing should be retried and all work
should be abandoned. I'm just advocating doing what can be done in the
driver code to avoid misleading unsophisticated calling code in
thinking that processing can continue somehow.

Florent

--
Florent Guillaume, Director of R&D, Nuxeo
Open Source, Java EE based, Enterprise Content Management (ECM)
http://www.nuxeo.com   http://www.nuxeo.org   +33 1 40 33 79 87