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

On Fri, Aug 23, 2013 at 7:38 AM, Florent Guillaume <fg(at)nuxeo(dot)com> wrote:

> Hi,
>
> On Thu, Aug 15, 2013 at 9:07 AM, Andreas Rudolph
> <andreas(dot)rudolph(at)spontech-spine(dot)com> wrote:
> > Am 13.08.2013 um 19:15 schrieb Florent Guillaume <fg(at)nuxeo(dot)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 waiting for a Connection to be returned.
> In this situation there is no Connection at all, yet, that could be reused
> or removed from 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, release resources 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 client code and which executes
> Driver.connect(), is up to the programmer of the client code. Therefore I
> was asking to improve interoperability 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 that no (usable) Connection
> object could have been returned otherwise. I can't see any evidence in your
> reasoning why supplying an InterruptedException as a cause of a checked
> exception would do any harm here. It just reveals the fact that the
> 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
>
>
> -For better or worse I just pushed in a change which implements Florent's
> suggestions.
>
>
>
> Dave Cramer
>
> dave.cramer(at)credativ(dot)ca
> http://www.credativ.ca
>
>