Close all connections on driver.close() #252
Conversation
| */ | ||
| public boolean offer( PooledConnection pooledConnection ) | ||
| { | ||
| boolean offer = queue.offer( pooledConnection ); |
There was a problem hiding this comment.
Is it necessary to synchronize here, i.e. making the move from queue to acquiredConnections atomic? It feels like you should but I am failing to come up with a scenario where it matters, so I opted for less synchronization.
There was a problem hiding this comment.
The order should be first remove from acquiredConn, then put back to queue to avid the following scenario happening:
T1: queue.offer(connA)
T2: acquire() -> get connA // connA = queue.poll, acquiredConn.Add(connA)
T1: acquiredConn.remove(connA)
NOTE: Remove first, then add back. This rule applies to acquire too
There was a problem hiding this comment.
Also, whenever you add into the queue/acquiredConn, you probably want to test if the pool already terminated. if it is already terminated, then drop the conn directly.
There was a problem hiding this comment.
Ref: https://github.com/neo4j/neo4j-dotnet-driver/blob/1.1/Neo4j.Driver/Neo4j.Driver/Internal/ConnectionPool.cs
We used lock because we do not have a concurrent container before. We probably could have a lock free impl for java driver
pontusmelke
force-pushed
the
1.1-close-conn-on-driver-close
branch
3 times, most recently
from
2cd6bec to
d2239b8
Compare
Connections/Sessions that were in flight, i.e. acquired from the pool, were not closed when shutting down the driver, connections remained open until GC happened. The expected behavior is that `driver.close()` closes down all sessions it has created.
pontusmelke
force-pushed
the
1.1-close-conn-on-driver-close
branch
from
d2239b8 to
8cfe635
Compare
| */ | ||
| public boolean offer( PooledConnection pooledConnection ) | ||
| { | ||
| boolean offer = queue.offer( pooledConnection ); |
There was a problem hiding this comment.
The order should be first remove from acquiredConn, then put back to queue to avid the following scenario happening:
T1: queue.offer(connA)
T2: acquire() -> get connA // connA = queue.poll, acquiredConn.Add(connA)
T1: acquiredConn.remove(connA)
NOTE: Remove first, then add back. This rule applies to acquire too
| */ | ||
| public boolean offer( PooledConnection pooledConnection ) | ||
| { | ||
| boolean offer = queue.offer( pooledConnection ); |
There was a problem hiding this comment.
Also, whenever you add into the queue/acquiredConn, you probably want to test if the pool already terminated. if it is already terminated, then drop the conn directly.
| * Terminates all connections, both those that are currently in the queue as well | ||
| * as those that have been acquired. | ||
| */ | ||
| public void terminate() |
There was a problem hiding this comment.
You probably want a atomic var here at the very beginning of the method, to flag it that we've started termination. If any connection created after this, close it right now. If any connection want to add to the queue or map, drop them directly.
| @@ -139,7 +139,7 @@ public synchronized void reset() | |||
| @Override | |||
| public boolean isOpen() | |||
| { | |||
| return isOpen.get(); | |||
| return isOpen.get() && connection.isOpen(); | |||
There was a problem hiding this comment.
I am not sure if we should add connection.isOpen test here. Though it is handle, however this method before is just to test if session.close is ever called. If you want to change this to if this session could run more queries or something else, then make sure that other methods who calling this isOpen is not broken.
| */ | ||
| public boolean offer( PooledConnection pooledConnection ) | ||
| { | ||
| boolean offer = queue.offer( pooledConnection ); |
There was a problem hiding this comment.
Ref: https://github.com/neo4j/neo4j-dotnet-driver/blob/1.1/Neo4j.Driver/Neo4j.Driver/Internal/ConnectionPool.cs
We used lock because we do not have a concurrent container before. We probably could have a lock free impl for java driver
pontusmelke
force-pushed
the
1.1-close-conn-on-driver-close
branch
from
22762d2 to
8faa0fd
Compare
pontusmelke
force-pushed
the
1.1-close-conn-on-driver-close
branch
from
8faa0fd to
f7c4c18
Compare
Connections/Sessions that were in flight, i.e. acquired from the pool, were not
closed when shutting down the driver, connections remained open until GC
happened. The expected behavior is that
driver.close()closes down allsessions it has created.