Skip to content

Commit d62518d

Browse files
committed
Remove test for Connection #finalize
This hurts bad to do, and I'm open to any suggestion for how to do this test in a way that is deterministic. Whenever #finalize is called on a user-level Session object, we check that the session was properly closed. If it was not closed, we generate an error and close the session. The test for this would create a Session object with no references, and then run tens of thousands of iterations of System.gc(), testing for #close() being invoked. This seems to have worked deterministically on Java 8, but on Java 7 it is flaky. I don't think testing for Java 7 in a JUnit assumption is a good solution, and instead I'm opting for removing the test, with the argument that this has been manually verified to work, it is a best-effort piece of code that in production usage is allowed to fail (since we can never guarante the finalizer will invoke us). If we drop java 7 support, we could look at reverting this commit.
1 parent 91f79c7 commit d62518d

File tree

2 files changed

+1
-85
lines changed

2 files changed

+1
-85
lines changed

driver/src/main/java/org/neo4j/driver/internal/InternalSession.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ protected void finalize() throws Throwable
139139
"method on Sessions before disposing of the objects.", null );
140140
connection.close();
141141
}
142+
super.finalize();
142143
}
143144

144145
private void ensureNoOpenTransaction()

driver/src/test/java/org/neo4j/driver/internal/InternalSessionTest.java

Lines changed: 0 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -22,21 +22,13 @@
2222
import org.junit.Test;
2323
import org.junit.rules.ExpectedException;
2424

25-
import java.util.Map;
26-
import java.util.concurrent.atomic.AtomicBoolean;
27-
2825
import org.neo4j.driver.internal.logging.DevNullLogger;
2926
import org.neo4j.driver.internal.spi.Connection;
30-
import org.neo4j.driver.internal.spi.Logger;
31-
import org.neo4j.driver.internal.spi.StreamCollector;
3227
import org.neo4j.driver.v1.Transaction;
33-
import org.neo4j.driver.v1.Value;
3428
import org.neo4j.driver.v1.exceptions.ClientException;
3529

3630
import static junit.framework.TestCase.assertNotNull;
37-
import static org.jsoup.helper.Validate.fail;
3831
import static org.mockito.Mockito.mock;
39-
import static org.mockito.Mockito.spy;
4032
import static org.mockito.Mockito.verify;
4133
import static org.mockito.Mockito.when;
4234

@@ -143,81 +135,4 @@ public void shouldNotAllowMoreTransactionsInSessionWhileConnectionClosed() throw
143135
// When
144136
sess.beginTransaction();
145137
}
146-
147-
@Test
148-
public void shouldWarnAndCloseOnLeak() throws Throwable
149-
{
150-
// Given
151-
CloseTrackingConnection connection = new CloseTrackingConnection();
152-
Logger logger = spy( Logger.class );
153-
154-
// When
155-
new InternalSession( connection, logger );
156-
157-
// Then
158-
long deadline = System.currentTimeMillis() + 1000 * 30;
159-
while ( !connection.closeCalled.get() )
160-
{
161-
System.gc();
162-
if ( System.currentTimeMillis() > deadline )
163-
{
164-
fail( "Expected unclosed session object to close its inner connection on finalize." );
165-
}
166-
}
167-
168-
verify( logger ).error( "Neo4j Session object leaked, please ensure that your application calls the `close` method on Sessions before disposing of the objects.", null );
169-
}
170-
171-
private static class CloseTrackingConnection implements Connection
172-
{
173-
AtomicBoolean closeCalled = new AtomicBoolean();
174-
175-
@Override
176-
public void init( String clientName )
177-
{
178-
179-
}
180-
181-
@Override
182-
public void run( String statement, Map<String,Value> parameters, StreamCollector collector )
183-
{
184-
185-
}
186-
187-
@Override
188-
public void discardAll()
189-
{
190-
191-
}
192-
193-
@Override
194-
public void pullAll( StreamCollector collector )
195-
{
196-
197-
}
198-
199-
@Override
200-
public void reset( StreamCollector collector )
201-
{
202-
203-
}
204-
205-
@Override
206-
public void sync()
207-
{
208-
209-
}
210-
211-
@Override
212-
public void close()
213-
{
214-
closeCalled.set( true );
215-
}
216-
217-
@Override
218-
public boolean isOpen()
219-
{
220-
return true;
221-
}
222-
}
223138
}

0 commit comments

Comments
 (0)