Better handling of concurrent driver close #291
Conversation
It is possible for connection to be closed from another thread while it is being used to do some work (send/receive data from socket, etc.). This could happen when whole driver is closed while there still exist threads doing actual work. Each `SocketConnection` is guarded by `ConcurrencyGuardingConnection` that throws if concurrent access is detected. This wrapper guarded `#close()` call as well so exceptions could be thrown on `Driver#close()`. Close procedure would then not be completed and some connections would not be closed. This commit allows concurrent close in `ConcurrencyGuardingConnection`.
`BlockingPooledConnectionQueue` disposes both idle and acquired connections when it is terminated. It was previously possible for termination process to stop halfway if disposal of some connection fails. This commit makes pool termination try to dispose all connections regardless of errors.
This pool can be used from multiple threads. It is possible for `#close()` to be called concurrently with `#acquire()`. Previously we did not guard against this situation and it was possible to acquire connections from the closed pool. It could also happen that `#close()` does not actually close all connection queues because iteration over them is not atomic. This commit makes `SocketConnectionPool` track if it is closed or not. It also introduces `Connector` abstraction (which is merely a connection factory) so that `SocketConnectionPool` can be easily tested.
Init message is send every time we establish a new socket connection. This is the very first thing connection does. It is possible for the init to fail (for example when credentials are wrong). Previously socket connection was not closed after such init failure. This commit changes `SocketConnector` to close connection on any init failure.
Connection pool (`SocketConnectionPool`) is created before drivers and was never closed if driver creation failed. This was especially possible with `RoutingDriver` which tries to build routing table in constructor. This commit adds closing of the connection pool when creation of driver fails. It also extracts driver creation logic into an internal class `DriverFactory` to make it testable.
| @@ -64,10 +69,10 @@ public boolean offer( PooledConnection pooledConnection ) | |||
| pooledConnection.dispose(); | |||
| } | |||
| if (isTerminating.get()) { | |||
| PooledConnection poll = queue.poll(); | |||
| if (poll != null) | |||
| PooledConnection connection = queue.poll(); | |||
| { | ||
| return insecure(); | ||
| } | ||
| return new DriverFactory().newInstance( uri, authToken, config.routingSettings(), config ); |
There was a problem hiding this comment.
I don't know, how about a static factory instance or a static newInstance method? Feels strange to create a stateless Factory object every time, but maybe you have a reason...
There was a problem hiding this comment.
Good point. Will get a new PR to fix this if possible
There was a problem hiding this comment.
DriverFactory class has protected methods so classes that extend it can "inject" different behaviour. Thus it is not possible to make #newInstance() static. Allocation of DriverFactory might also be eliminated by the JVM if GraphDatabase#driver() is called often.
| catch ( Exception e ) | ||
| { | ||
| assertThat( e, instanceOf( ClientException.class ) ); | ||
| } |
There was a problem hiding this comment.
One can write a quite useful assert helper for checking exceptions, see org.neo4j.test.assertion.Assert.assertException. If you're doing this often.
There was a problem hiding this comment.
I am generally against to use exception.expect as it is too hard to tell which line the exception is really thrown. When using it, from the line the exception is declared, any line afterwards could be the place where the exception is thrown.
There was a problem hiding this comment.
Unfortunately we do not have dependencies on artifacts from neo4j/neo4j.
There was a problem hiding this comment.
Yeah, I was thinking to simply copy paste that one assert method into some test-util class if you find it useful. Just FYI though, do as you like =).
No description provided.