Merge pull request #167 from jakewins/1.0-pool-close

jakewins · jakewins · commit e523912a1b04 · 2016-04-22T14:38:54.000+02:00
Resolve issue where pooled connections could leak if exception occurred
diff --git a/driver/src/main/java/org/neo4j/driver/internal/pool/PooledConnection.java b/driver/src/main/java/org/neo4j/driver/internal/pool/PooledConnection.java
@@ -156,12 +156,15 @@ public void close()
         {
             reset( StreamCollector.NO_OP );
             sync();
-            release.accept( this );
         }
         catch (Exception ex)
         {
             dispose();
         }
+        finally
+        {
+            release.accept( this );
+        }
     }
 
     @Override
diff --git a/driver/src/test/java/org/neo4j/driver/internal/pool/PooledConnectionTest.java b/driver/src/test/java/org/neo4j/driver/internal/pool/PooledConnectionTest.java
@@ -0,0 +1,64 @@
+/**
+ * Copyright (c) 2002-2016 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [https://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.driver.internal.pool;
+
+import org.junit.Test;
+
+import java.util.LinkedList;
+
+import org.neo4j.driver.internal.spi.Connection;
+import org.neo4j.driver.internal.spi.StreamCollector;
+import org.neo4j.driver.internal.util.Consumer;
+import org.neo4j.driver.v1.exceptions.DatabaseException;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.hasItem;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+
+public class PooledConnectionTest
+{
+    @Test
+    public void shouldReturnToPoolIfExceptionDuringReset() throws Throwable
+    {
+        // Given
+        final LinkedList<PooledConnection> returnedToPool = new LinkedList<>();
+        Connection conn = mock( Connection.class );
+
+        doThrow( new DatabaseException( "asd", "asd" ) ).when(conn).reset( any( StreamCollector.class) );
+
+        PooledConnection pooledConnection = new PooledConnection( conn, new Consumer<PooledConnection>()
+        {
+            @Override
+            public void accept( PooledConnection pooledConnection )
+            {
+                returnedToPool.add( pooledConnection );
+            }
+        } );
+
+        // When
+        pooledConnection.close();
+
+        // Then
+        assertThat( returnedToPool, hasItem(pooledConnection) );
+        assertThat( returnedToPool.size(), equalTo( 1 ));
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -156,12 +156,15 @@ public void close()`
`156`	`156`	`{`
`157`	`157`	`reset( StreamCollector.NO_OP );`
`158`	`158`	`sync();`
`159`		`- release.accept( this );`
`160`	`159`	`}`
`161`	`160`	`catch (Exception ex)`
`162`	`161`	`{`
`163`	`162`	`dispose();`
`164`	`163`	`}`
	`164`	`+ finally`
	`165`	`+ {`
	`166`	`+ release.accept( this );`
	`167`	`+ }`
`165`	`168`	`}`
`166`	`169`
`167`	`170`	`@Override`