Handle: unconditionally close Connection

[stevenschlansker]

Right now, we check if a connection is closed before closing it.

However, this isn't good enough: even if the server closed the connection, we might have borrowed this from a pool, which needs to know we've returned it.

Connection.close is specified to no-op if called more than once, so it's safe to just do it regardless.

Fixes #2446

[hgschmie]

This might need a bit more thinking,. It is one of these "damned if I do, damned if I don't" situation because of the plethora of drivers. The check exists because there are a number of drivers that throw a "ConnectionAlreadyClosedException" if calling Connection#close on a closed connection (yes, it's against the spec, but but ...). Making the close unconditional might pop up more errors in more popular paths.

The isAlive check was ported over in the 3.35.0 rewrite from the previous code.

I can take a closer look at the change over the weekend when I am no longer in an airport departure lounge, please hold off merging till then.

The bug report worries me. It implies that there are connection pools that report a connection "dead" (IAW closed) while still holding onto the actual database driver connection. This would be a pool bug.

[stevenschlansker]

No urgency to merge this.

Throwing a ConnectionAlreadyClosedException is explicitly violating the spec. Can you be more specific about which driver(s) do this?

I think it is understandable that if the pool leases a connection, it expects it to be returned, even if the database server asynchronously-closes the connection. Maybe there is a pool bug but I am not as sure as you are.

[stevenschlansker]

If drivers violating the spec is a problem, we can catch and ignore the thrown exception. Checking the connection isValid isn't a proper way to avoid this anyway, since it leaves open a race where the connection is closed behind your back between the valid check and doing the close.

[kristoffSC]

Hi,
Thanks for looking at this, I'm an author of that bug report.

Regarding:

It implies that there are connection pools that report a connection "dead" (IAW closed) while still holding onto the actual database driver connection. This would be a pool bug.

vs

I think it is understandable that if the pool leases a connection, it expects it to be returned

I think that the latter is more accurate for my bug report.
The bottom line is that jdbi will not return borrowed object to the pool. It's up to the pool what should be done with such object.

In my case, the connection was never returned to the pool. The pool can have "health" checks while creating, borrowing or returning object to the pool but in this case this would not work since connection was closed after it was acquired from the pool and before Cleanable was registered on the Handle object which happens in jdbi.

[hgschmie]

If drivers violating the spec is a problem, we can catch and ignore the thrown exception. Checking the connection isValid isn't a proper way to avoid this anyway, since it leaves open a race where the connection is closed behind your back between the valid check and doing the close.

Yeah, but by doing so, we now penalize all the code that behaves correctly (reporting the connection as dead when it is dead). Catching the exception is expensive.

This seems to be a case where we should just have a config switch. If the underlying pool/thing requires an unconditional close, you can set a flag and we force that.

[hgschmie]

Hi, Thanks for looking at this, I'm an author of that bug report.

Regarding:

It implies that there are connection pools that report a connection "dead" (IAW closed) while still holding onto the actual database driver connection. This would be a pool bug.

vs

I think it is understandable that if the pool leases a connection, it expects it to be returned

I think that the latter is more accurate for my bug report. The bottom line is that jdbi will not return borrowed object to the pool. It's up to the pool what should be done with such object.

In my case, the connection was never returned to the pool. The pool can have "health" checks while creating, borrowing or returning object to the pool but in this case this would not work since connection was closed after it was acquired from the pool and before Cleanable was registered on the Handle object which happens in jdbi.

Thank you for the explanation but that is then a pool bug. We call "checkConnectionLive()" which needs to return true so that we close the connection. This code calls Connection#isClosed() which is explicitly defined as A connection is closed if the method close has been called on it or if certain fatal errors have occurred. This method is guaranteed to return true only when it is called after the method Connection.close has been called.. So the pool connection returned "I am closed, no need to do anything", which then was not correct. This is not a Jdbi bug, but a pool bug.

Is there a specific pool library that shows that behavior? (please don't say "c3p0").

[hgschmie]

this makes me super-uncomfortable. There is a single bug report about a problem with connection closing and this is a change at one of the most fundamental pieces of Jdbi, affecting the most fundamental path in the whole code base which is probably executed a billion times a day all over the java ecosystem without any problems.

Why touch this and if touching it, there needs to be a bunch of tests that prove that this low level code (this replaces one line of straightforward catch and rethrow with a bunch of manual resource manipulations) actually is an improvement. I don't believe it is.

[hgschmie]

I spend a lot of time and wrote a lot of tests in the 3.35.0 cycle to prove that the new code not only behaves better but does not leak resources anymore. I don't think introducing this manual piece-by-piece resource releasing and bypassing the mechanism that is used everywhere else in post-3.35.0 code is the right thing to do.

[kristoffSC]

Hi @hgschmie
I totally understand your concerns.

There is a single bug report about a problem with connection closing

This is true, it is a single bug report and it was originally about connection closing. However yesterday I have updated the bug report because it turns out that the problem that I found is more generic and case when db connection was closed can be one of the "implementation" of this issue.

The pool has basically two sets of methods,
one for acquiring the object and second one for returning/destroying the object when it is no longer needed. Both methods are called by pool client which in this case is JDBI. It is the pool client who knows when to return the object back to the pool, so its up to the caller (JDBI i this case) to make sure that object is always return back to the pool right? At lest to make sure that proper API is called. Forgive me but I can not say that this is a pool bug, but maybe I'm missing something.

The org.jdbi.v3.core.Jdbi::open() gets object from the pool. If there will be an exception thrown anywhere in org.jdbi.v3.core.Jdbi::open() method, after the connection was acquired from pool, the connection object will not be returned back to the pool.

I would say that org.jdbi.v3.core.Jdbi::open() should have exception handling logic which will guaranty that connection object will be properly returned to the pool in case of exception. Currently it is not the case.

The specific use cases for connection close:

1. The connection is closed from the client side, as described in this bug report.
   In this case it turns out that exception will be thrown by this line in Handle's constructor.
   this.transactionHandler = transactionHandler.specialize(this); so even before registering any Cleanable objects
   In this case, since exception was thrown from Handle constructor, there is no Handle instance to close/clean at the end.

2. The second use case is where the connection is closed from server side, for example Data Base restart.
   In our system we are using Jdbi's PostgresPlugin. In org.jdbi.v3.core.Jdbi::open() method, after Handle object is created there is this call:

for (JdbiPlugin p : plugins) {
        h = p.customizeHandle(h);
}

This will throw an exception for PostgresPlugin.
In result the open() method will not return any Handle reference so no Handle will be closed/clean up hence connection object will not be returned to the pool. The stack trace for this case -
JdbiTrace.txt

To reproduce this, please pull my repository (I added PostgresPlugin) and simply restart Postgresql container on a break point just after where connection object is acquired from the pool.

However this problem will happen in case of any exception thrown in Jdbi::open() method. The connection issue is only one example (that I could find/reproduce on a real system).

[stevenschlansker]

I added test cases to show this behavior in a unit test that we can commit to the repo.

[kristoffSC]

Hi @hgschmie

Thank you for the explanation but that is then a pool bug. We call "checkConnectionLive()" which needs to return true so that we close the connection. This code calls Connection#isClosed() which is explicitly defined as A connection is closed if the method close has been called on it or if certain fatal errors have occurred. This method is guaranteed to return true only when it is called after the method Connection.close has been called.. So the pool connection returned "I am closed, no need to do anything", which then was not correct. This is not a Jdbi bug, but a pool bug.

Not sure if I can agree with that this is pool bug. Originally bug report was about connection closing, but after further investigation I believe that there is an issue with exception handling in org.jdbi.v3.core.Jdbi::open(). the bug report was updated. I believe that its up to the client code (JDBI code) to make sure or at least try to return borrowed object to the pool in any case. Please see my comment here for details: #2451 (comment)

Is there a specific pool library that shows that behavior? (please don't say "c3p0").
I'm using org.apache.commons.dbcp2.PoolingDataSource wrapping org.apache.commons.pool2.impl.GenericObjectPool
My JDBI setup looks like this:

BasicDataSource dataSource = new BasicDataSource();
    dataSource.setUrl(container.getJdbcUrl());
    dataSource.setPassword("password");
    dataSource.setUsername("user");
    dataSource.setMinIdle(2);
    dataSource.setMaxIdle(4);
    dataSource.setMaxTotal(6);

    this.jdbi = Jdbi.create(dataSource)
        .installPlugin(new SqlObjectPlugin())
        .installPlugin(new PostgresPlugin());

[stevenschlansker]

Yeah, but by doing so, we now penalize all the code that behaves correctly (reporting the connection as dead when it is dead). Catching the exception is expensive.

I think this is the other way around: the current code penalizes all code that behaves correctly, because calling isClosed does a lot of work (in Pg, it will require a server roundtrip), and we can always safely close without that roundtrip. The Pg code is careful that calling close() a second time is very cheap. If there are other concrete databases that behave differently, we can talk about that, but we need a specific example else we should go with the spec which says "don't use isClosed since it is not a reliable test; close is safe to call multiple times". They specifically call out not to rely on it.

Throwing an exception on duplicate close is forbidden by spec, so it will not happen, so it has no cost. Unless there's a specific case of a driver that incorrectly does this.

This seems to be a case where we should just have a config switch. If the underlying pool/thing requires an unconditional close, you can set a flag and we force that.

If we have a specific database where this is needed, we can add it. I don't think we should pre-emptively add configuration in contradiction to the JDBC spec without a specific reason. That just adds complexity.

I agree with Kristoff that we have a potential leak in Jdbi.open, if e.g. a plugin customizeConnection throws, we never return the Handle nor close the Connection.

[stevenschlansker]

The changes you pushed over mine undoes the cleanup of resources. So I restored that.

I wrote two tests: one to show the leak when customization fails, and one to show the leak when isClosed throws. Both tests fail in master today, and pass with this fix.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[stevenschlansker]

closed in favor of #2458