From 5a4c9e215f736fd3c5f3050a1e2d3e18f97f783d Mon Sep 17 00:00:00 2001 From: lutovich Date: Wed, 30 Nov 2016 23:03:27 +0000 Subject: [PATCH 1/2] Removed finalize from NetworkSession It contained logic for closing opened underlying connection and logging info about the leak. Closing of resources in finalize is not reliable and overridden finalize can hurt when many sessions are garbage collected. Commit also introduces a system property `org.neo4j.driver.logSessionLeaks` to enable logging of unclosed/leaked sessions and log stacktrace of where they were instantiated. This is meant to help with session leak investigations. So finalization overhead will be present only when problem is present and is being investigated. --- .../org/neo4j/driver/internal/BaseDriver.java | 4 +- .../neo4j/driver/internal/DirectDriver.java | 5 +- .../neo4j/driver/internal/DriverFactory.java | 34 +++++-- .../internal/LeakLoggingNetworkSession.java | 65 +++++++++++++ .../LeakLoggingNetworkSessionFactory.java | 49 ++++++++++ .../neo4j/driver/internal/NetworkSession.java | 15 +-- .../internal/NetworkSessionFactory.java | 31 +++++++ .../neo4j/driver/internal/RoutingDriver.java | 5 +- .../neo4j/driver/internal/SessionFactory.java | 27 ++++++ .../driver/internal/DriverFactoryTest.java | 4 +- .../LeakLoggingNetworkSessionFactoryTest.java | 42 +++++++++ .../LeakLoggingNetworkSessionTest.java | 92 +++++++++++++++++++ .../internal/NetworkSessionFactoryTest.java | 41 +++++++++ .../driver/internal/RoutingDriverTest.java | 3 +- 14 files changed, 386 insertions(+), 31 deletions(-) create mode 100644 driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSession.java create mode 100644 driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java create mode 100644 driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java create mode 100644 driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactoryTest.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionTest.java create mode 100644 driver/src/test/java/org/neo4j/driver/internal/NetworkSessionFactoryTest.java diff --git a/driver/src/main/java/org/neo4j/driver/internal/BaseDriver.java b/driver/src/main/java/org/neo4j/driver/internal/BaseDriver.java index e021aa6232..9519939c80 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/BaseDriver.java +++ b/driver/src/main/java/org/neo4j/driver/internal/BaseDriver.java @@ -32,13 +32,15 @@ abstract class BaseDriver implements Driver private final static String DRIVER_LOG_NAME = "Driver"; private final SecurityPlan securityPlan; + protected final SessionFactory sessionFactory; protected final Logger log; private AtomicBoolean closed = new AtomicBoolean( false ); - BaseDriver( SecurityPlan securityPlan, Logging logging ) + BaseDriver( SecurityPlan securityPlan, SessionFactory sessionFactory, Logging logging ) { this.securityPlan = securityPlan; + this.sessionFactory = sessionFactory; this.log = logging.getLog( DRIVER_LOG_NAME ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/DirectDriver.java b/driver/src/main/java/org/neo4j/driver/internal/DirectDriver.java index 8fd6f159c2..80209a0e97 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DirectDriver.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DirectDriver.java @@ -36,9 +36,10 @@ public DirectDriver( BoltServerAddress address, ConnectionPool connections, SecurityPlan securityPlan, + SessionFactory sessionFactory, Logging logging ) { - super( securityPlan, logging ); + super( securityPlan, sessionFactory, logging ); this.address = address; this.connections = connections; } @@ -46,7 +47,7 @@ public DirectDriver( @Override protected Session newSessionWithMode( AccessMode mode ) { - return new NetworkSession( connections.acquire( address ) ); + return sessionFactory.newInstance( connections.acquire( address ) ); } @Override diff --git a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java index 022a456947..44f75310ad 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java @@ -49,10 +49,13 @@ public final Driver newInstance( URI uri, AuthToken authToken, RoutingSettings r BoltServerAddress address = BoltServerAddress.from( uri ); SecurityPlan securityPlan = createSecurityPlan( address, config ); ConnectionPool connectionPool = createConnectionPool( authToken, securityPlan, config ); + SessionFactory sessionFactory = createSessionFactory( config ); try { - return createDriver( address, uri.getScheme(), connectionPool, config, routingSettings, securityPlan ); + return createDriver( address, uri.getScheme(), connectionPool, config, routingSettings, securityPlan, + sessionFactory + ); } catch ( Throwable driverError ) { @@ -70,14 +73,16 @@ public final Driver newInstance( URI uri, AuthToken authToken, RoutingSettings r } private Driver createDriver( BoltServerAddress address, String scheme, ConnectionPool connectionPool, - Config config, RoutingSettings routingSettings, SecurityPlan securityPlan ) + Config config, RoutingSettings routingSettings, SecurityPlan securityPlan, + SessionFactory sessionFactory ) { switch ( scheme.toLowerCase() ) { case "bolt": - return createDirectDriver( address, connectionPool, config, securityPlan ); + return createDirectDriver( address, connectionPool, config, securityPlan, sessionFactory ); case "bolt+routing": - return createRoutingDriver( address, connectionPool, config, routingSettings, securityPlan ); + return createRoutingDriver( address, connectionPool, config, routingSettings, securityPlan, + sessionFactory ); default: throw new ClientException( format( "Unsupported URI scheme: %s", scheme ) ); } @@ -89,9 +94,9 @@ private Driver createDriver( BoltServerAddress address, String scheme, Connectio * This method is package-private only for testing */ DirectDriver createDirectDriver( BoltServerAddress address, ConnectionPool connectionPool, Config config, - SecurityPlan securityPlan ) + SecurityPlan securityPlan, SessionFactory sessionFactory ) { - return new DirectDriver( address, connectionPool, securityPlan, config.logging() ); + return new DirectDriver( address, connectionPool, securityPlan, sessionFactory, config.logging() ); } /** @@ -99,11 +104,11 @@ DirectDriver createDirectDriver( BoltServerAddress address, ConnectionPool conne *

* This method is package-private only for testing */ - RoutingDriver createRoutingDriver( BoltServerAddress address, ConnectionPool connectionPool, - Config config, RoutingSettings routingSettings, SecurityPlan securityPlan ) + RoutingDriver createRoutingDriver( BoltServerAddress address, ConnectionPool connectionPool, Config config, + RoutingSettings routingSettings, SecurityPlan securityPlan, SessionFactory sessionFactory ) { - return new RoutingDriver( routingSettings, address, connectionPool, securityPlan, Clock.SYSTEM, - config.logging() ); + return new RoutingDriver( routingSettings, address, connectionPool, securityPlan, sessionFactory, + Clock.SYSTEM, config.logging() ); } /** @@ -122,6 +127,15 @@ ConnectionPool createConnectionPool( AuthToken authToken, SecurityPlan securityP return new SocketConnectionPool( poolSettings, connector, Clock.SYSTEM, config.logging() ); } + private static SessionFactory createSessionFactory( Config config ) + { + if ( LeakLoggingNetworkSessionFactory.leakLoggingEnabled() ) + { + return new LeakLoggingNetworkSessionFactory( config.logging() ); + } + return new NetworkSessionFactory(); + } + private static SecurityPlan createSecurityPlan( BoltServerAddress address, Config config ) { try diff --git a/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSession.java b/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSession.java new file mode 100644 index 0000000000..a74352a686 --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSession.java @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Logger; + +import static java.lang.System.lineSeparator; + +class LeakLoggingNetworkSession extends NetworkSession +{ + private final Logger log; + private final String stackTrace; + + LeakLoggingNetworkSession( Connection connection, Logger log ) + { + super( connection ); + this.log = log; + this.stackTrace = captureStackTrace(); + } + + @Override + protected void finalize() throws Throwable + { + logLeakIfNeeded(); + super.finalize(); + } + + private void logLeakIfNeeded() + { + if ( isOpen() ) + { + log.error( "Neo4j Session object leaked, please ensure that your application" + + "calls the `close` method on Sessions before disposing of the objects.\n" + + "Session was create at:\n" + stackTrace, null ); + } + } + + private static String captureStackTrace() + { + StringBuilder result = new StringBuilder(); + StackTraceElement[] elements = Thread.currentThread().getStackTrace(); + for ( StackTraceElement element : elements ) + { + result.append( "\t" ).append( element ).append( lineSeparator() ); + } + return result.toString(); + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java new file mode 100644 index 0000000000..aa6a1142d3 --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Logger; +import org.neo4j.driver.v1.Logging; +import org.neo4j.driver.v1.Session; + +public class LeakLoggingNetworkSessionFactory implements SessionFactory +{ + private static final String SYSTEM_PROPERTY_NAME = "org.neo4j.driver.logSessionLeaks"; + private static final String LOGGER_NAME = "sessionLeak"; + + private final Logger logger; + + public LeakLoggingNetworkSessionFactory( Logging logging ) + { + this.logger = logging.getLog( LOGGER_NAME ); + } + + public static boolean leakLoggingEnabled() + { + String value = System.getProperty( SYSTEM_PROPERTY_NAME ); + return Boolean.parseBoolean( value ); + } + + @Override + public Session newInstance( Connection connection ) + { + return new LeakLoggingNetworkSession( connection, logger ); + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java b/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java index bdd0beff3c..93798e79c8 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java +++ b/driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java @@ -67,7 +67,7 @@ public void run() private ExplicitTransaction currentTransaction; private AtomicBoolean isOpen = new AtomicBoolean( true ); - public NetworkSession( Connection connection ) + NetworkSession( Connection connection ) { this.connection = connection; @@ -126,6 +126,7 @@ public static StatementResult run( Connection connection, Statement statement ) return cursor; } + @Override public synchronized void reset() { ensureSessionIsOpen(); @@ -255,18 +256,6 @@ private void ensureConnectionIsValidBeforeOpeningTransaction() ensureConnectionIsOpen(); } - @Override - protected void finalize() throws Throwable - { - if ( isOpen.compareAndSet( true, false ) ) - { - logger.error( "Neo4j Session object leaked, please ensure that your application calls the `close` " + - "method on Sessions before disposing of the objects.", null ); - connection.close(); - } - super.finalize(); - } - private void ensureNoUnrecoverableError() { if ( connection.hasUnrecoverableErrors() ) diff --git a/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java new file mode 100644 index 0000000000..04fb2990fb --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Session; + +public class NetworkSessionFactory implements SessionFactory +{ + @Override + public Session newInstance( Connection connection ) + { + return new NetworkSession( connection ); + } +} diff --git a/driver/src/main/java/org/neo4j/driver/internal/RoutingDriver.java b/driver/src/main/java/org/neo4j/driver/internal/RoutingDriver.java index e21197558d..ec91399fed 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/RoutingDriver.java +++ b/driver/src/main/java/org/neo4j/driver/internal/RoutingDriver.java @@ -51,10 +51,11 @@ public RoutingDriver( BoltServerAddress seedAddress, ConnectionPool connections, SecurityPlan securityPlan, + SessionFactory sessionFactory, Clock clock, Logging logging ) { - super( verifiedSecurityPlan( securityPlan ), logging ); + super( verifiedSecurityPlan( securityPlan ), sessionFactory, logging ); this.loadBalancer = new LoadBalancer( settings, clock, log, connections, seedAddress ); } @@ -62,7 +63,7 @@ public RoutingDriver( protected Session newSessionWithMode( AccessMode mode ) { Connection connection = acquireConnection( mode ); - NetworkSession networkSession = new NetworkSession( connection ); + Session networkSession = sessionFactory.newInstance( connection ); return new RoutingNetworkSession( networkSession, mode, connection.boltServerAddress(), loadBalancer ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java new file mode 100644 index 0000000000..7f48a53416 --- /dev/null +++ b/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Session; + +public interface SessionFactory +{ + Session newInstance( Connection connection ); +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java b/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java index f96b9762f6..3530273163 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java @@ -121,14 +121,14 @@ private static class ThrowingDriverFactory extends DriverFactory @Override DirectDriver createDirectDriver( BoltServerAddress address, ConnectionPool connectionPool, Config config, - SecurityPlan securityPlan ) + SecurityPlan securityPlan, SessionFactory sessionFactory ) { throw new UnsupportedOperationException( "Can't create direct driver" ); } @Override RoutingDriver createRoutingDriver( BoltServerAddress address, ConnectionPool connectionPool, Config config, - RoutingSettings routingSettings, SecurityPlan securityPlan ) + RoutingSettings routingSettings, SecurityPlan securityPlan, SessionFactory sessionFactory ) { throw new UnsupportedOperationException( "Can't create routing driver" ); } diff --git a/driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactoryTest.java b/driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactoryTest.java new file mode 100644 index 0000000000..0dccfb397c --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactoryTest.java @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.junit.Test; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Logging; +import org.neo4j.driver.v1.Session; + +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; + +public class LeakLoggingNetworkSessionFactoryTest +{ + @Test + public void createsLeakLoggingNetworkSessions() + { + SessionFactory factory = new LeakLoggingNetworkSessionFactory( mock( Logging.class ) ); + + Session session = factory.newInstance( mock( Connection.class ) ); + + assertThat( session, instanceOf( LeakLoggingNetworkSession.class ) ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionTest.java b/driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionTest.java new file mode 100644 index 0000000000..3970a9ae5a --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionTest.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestName; +import org.mockito.ArgumentCaptor; + +import java.lang.reflect.Method; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Logger; +import org.neo4j.driver.v1.Session; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; + +public class LeakLoggingNetworkSessionTest +{ + @Rule + public final TestName testName = new TestName(); + + @Test + public void logsNothingDuringFinalizationIfClosed() throws Exception + { + Logger logger = mock( Logger.class ); + Session session = new LeakLoggingNetworkSession( connectionMock( false ), logger ); + + finalize( session ); + + verifyZeroInteractions( logger ); + } + + @Test + public void logsMessageWithStacktraceDuringFinalizationIfLeaked() throws Exception + { + Logger logger = mock( Logger.class ); + Session session = new LeakLoggingNetworkSession( connectionMock( true ), logger ); + + finalize( session ); + + ArgumentCaptor messageCaptor = ArgumentCaptor.forClass( String.class ); + verify( logger ).error( messageCaptor.capture(), any( Throwable.class ) ); + + assertEquals( 1, messageCaptor.getAllValues().size() ); + + String loggedMessage = messageCaptor.getValue(); + assertThat( loggedMessage, startsWith( "Neo4j Session object leaked" ) ); + assertThat( loggedMessage, containsString( "Session was create at" ) ); + assertThat( loggedMessage, containsString( + getClass().getSimpleName() + "." + testName.getMethodName() ) + ); + } + + private static void finalize( Session session ) throws Exception + { + Method finalizeMethod = session.getClass().getDeclaredMethod( "finalize" ); + finalizeMethod.setAccessible( true ); + finalizeMethod.invoke( session ); + } + + private static Connection connectionMock( boolean open ) + { + Connection connection = mock( Connection.class ); + when( connection.isOpen() ).thenReturn( open ); + return connection; + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionFactoryTest.java b/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionFactoryTest.java new file mode 100644 index 0000000000..a4b6dadaa5 --- /dev/null +++ b/driver/src/test/java/org/neo4j/driver/internal/NetworkSessionFactoryTest.java @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://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 + * + * http://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; + +import org.junit.Test; + +import org.neo4j.driver.internal.spi.Connection; +import org.neo4j.driver.v1.Session; + +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; + +public class NetworkSessionFactoryTest +{ + @Test + public void createsNetworkSessions() + { + SessionFactory factory = new NetworkSessionFactory(); + + Session session = factory.newInstance( mock( Connection.class ) ); + + assertThat( session, instanceOf( NetworkSession.class ) ); + } +} diff --git a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java index dcbe6346b8..0ef10f1c71 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/RoutingDriverTest.java @@ -336,7 +336,8 @@ private final RoutingDriver driverWithServers( long ttl, Map... s private RoutingDriver driverWithPool( ConnectionPool pool ) { - return new RoutingDriver( new RoutingSettings( 10, 5_000 ), SEED, pool, insecure(), clock, logging ); + RoutingSettings settings = new RoutingSettings( 10, 5_000 ); + return new RoutingDriver( settings, SEED, pool, insecure(), new NetworkSessionFactory(), clock, logging ); } @SafeVarargs From b0ce429b57e6def2549e678b8ae4f675c9d595cd Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 12 Dec 2016 15:03:09 +0100 Subject: [PATCH 2/2] Move leaked session switch to config It was previously in system property. This commit makes it part of `Config` so we have a single place with all configuration parameters. --- .../neo4j/driver/internal/DriverFactory.java | 2 +- .../LeakLoggingNetworkSessionFactory.java | 11 +---- .../internal/NetworkSessionFactory.java | 2 +- .../neo4j/driver/internal/SessionFactory.java | 2 +- .../main/java/org/neo4j/driver/v1/Config.java | 35 +++++++++++++++ .../org/neo4j/driver/internal/ConfigTest.java | 12 ++++++ .../driver/internal/DriverFactoryTest.java | 43 +++++++++++++++++++ 7 files changed, 95 insertions(+), 12 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java index 44f75310ad..d74207d992 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java @@ -129,7 +129,7 @@ ConnectionPool createConnectionPool( AuthToken authToken, SecurityPlan securityP private static SessionFactory createSessionFactory( Config config ) { - if ( LeakLoggingNetworkSessionFactory.leakLoggingEnabled() ) + if ( config.logLeakedSessions() ) { return new LeakLoggingNetworkSessionFactory( config.logging() ); } diff --git a/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java index aa6a1142d3..cae02d86b4 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/LeakLoggingNetworkSessionFactory.java @@ -23,24 +23,17 @@ import org.neo4j.driver.v1.Logging; import org.neo4j.driver.v1.Session; -public class LeakLoggingNetworkSessionFactory implements SessionFactory +class LeakLoggingNetworkSessionFactory implements SessionFactory { - private static final String SYSTEM_PROPERTY_NAME = "org.neo4j.driver.logSessionLeaks"; private static final String LOGGER_NAME = "sessionLeak"; private final Logger logger; - public LeakLoggingNetworkSessionFactory( Logging logging ) + LeakLoggingNetworkSessionFactory( Logging logging ) { this.logger = logging.getLog( LOGGER_NAME ); } - public static boolean leakLoggingEnabled() - { - String value = System.getProperty( SYSTEM_PROPERTY_NAME ); - return Boolean.parseBoolean( value ); - } - @Override public Session newInstance( Connection connection ) { diff --git a/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java index 04fb2990fb..9dd96f3eda 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/NetworkSessionFactory.java @@ -21,7 +21,7 @@ import org.neo4j.driver.internal.spi.Connection; import org.neo4j.driver.v1.Session; -public class NetworkSessionFactory implements SessionFactory +class NetworkSessionFactory implements SessionFactory { @Override public Session newInstance( Connection connection ) diff --git a/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java b/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java index 7f48a53416..de77592b93 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java +++ b/driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java @@ -21,7 +21,7 @@ import org.neo4j.driver.internal.spi.Connection; import org.neo4j.driver.v1.Session; -public interface SessionFactory +interface SessionFactory { Session newInstance( Connection connection ); } diff --git a/driver/src/main/java/org/neo4j/driver/v1/Config.java b/driver/src/main/java/org/neo4j/driver/v1/Config.java index de0b7cd91d..ab177066e8 100644 --- a/driver/src/main/java/org/neo4j/driver/v1/Config.java +++ b/driver/src/main/java/org/neo4j/driver/v1/Config.java @@ -26,6 +26,7 @@ import org.neo4j.driver.internal.logging.JULogging; import org.neo4j.driver.internal.net.pooling.PoolSettings; import org.neo4j.driver.v1.util.Immutable; +import org.neo4j.driver.v1.util.Resource; import static org.neo4j.driver.v1.Config.TrustStrategy.trustAllCertificates; @@ -48,6 +49,7 @@ public class Config { /** User defined logging */ private final Logging logging; + private final boolean logLeakedSessions; private final int maxIdleConnectionPoolSize; @@ -63,6 +65,7 @@ public class Config private Config( ConfigBuilder builder) { this.logging = builder.logging; + this.logLeakedSessions = builder.logLeakedSessions; this.maxIdleConnectionPoolSize = builder.maxIdleConnectionPoolSize; @@ -81,6 +84,16 @@ public Logging logging() return logging; } + /** + * Check if leaked sessions logging is enabled. + * + * @return {@code true} if enabled, {@code false} otherwise. + */ + public boolean logLeakedSessions() + { + return logLeakedSessions; + } + /** * Max number of connections per URL for this driver. * @return the max number of connections @@ -158,6 +171,7 @@ RoutingSettings routingSettings() public static class ConfigBuilder { private Logging logging = new JULogging( Level.INFO ); + private boolean logLeakedSessions; private int maxIdleConnectionPoolSize = PoolSettings.DEFAULT_MAX_IDLE_CONNECTION_POOL_SIZE; private EncryptionLevel encryptionLevel = EncryptionLevel.REQUIRED; private TrustStrategy trustStrategy = trustAllCertificates(); @@ -178,6 +192,27 @@ public ConfigBuilder withLogging( Logging logging ) return this; } + /** + * Enable logging of leaked sessions. + *

+ * Each {@link Session session} is associated with a network connection and thus is a + * {@link Resource resource} that needs to be explicitly closed. Unclosed sessions will result in socket + * leaks and could cause {@link OutOfMemoryError}s. + *

+ * Session is considered to be leaked when it is finalized via {@link Object#finalize()} while not being + * closed. This option turns on logging of such sessions and stacktraces of where they were created. + *

+ * Note: this option should mostly be used in testing environments for session leak investigations. + * Enabling it will add object finalization overhead. + * + * @return this builder + */ + public ConfigBuilder withLeakedSessionsLogging() + { + this.logLeakedSessions = true; + return this; + } + /** * The max number of sessions to keep open at once. Configure this * higher if you want more concurrent sessions, or lower if you want diff --git a/driver/src/test/java/org/neo4j/driver/internal/ConfigTest.java b/driver/src/test/java/org/neo4j/driver/internal/ConfigTest.java index 8fc1e8f889..2786ed7942 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/ConfigTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/ConfigTest.java @@ -27,6 +27,8 @@ import static java.lang.System.getProperty; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class ConfigTest { @@ -86,6 +88,16 @@ public void shouldIgnoreLivenessCheckTimeoutSetting() throws Throwable assertEquals( -1, config.idleTimeBeforeConnectionTest() ); } + @Test + public void shouldTurnOnLeakedSessionsLogging() + { + // leaked sessions logging is turned off by default + assertFalse( Config.build().toConfig().logLeakedSessions() ); + + // it can be turned on using config + assertTrue( Config.build().withLeakedSessionsLogging().toConfig().logLeakedSessions() ); + } + public static void deleteDefaultKnownCertFileIfExists() { if( DEFAULT_KNOWN_HOSTS.exists() ) diff --git a/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java b/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java index 3530273163..5db00d555f 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java +++ b/driver/src/test/java/org/neo4j/driver/internal/DriverFactoryTest.java @@ -100,6 +100,28 @@ public void connectionPoolCloseExceptionIsSupressedWhenDriverCreationFails() thr verify( connectionPool ).close(); } + @Test + public void usesStandardSessionFactoryWhenNothingConfigured() + { + Config config = Config.defaultConfig(); + SessionFactoryCapturingDriverFactory factory = new SessionFactoryCapturingDriverFactory(); + + factory.newInstance( uri, dummyAuthToken(), dummyRoutingSettings(), config ); + + assertThat( factory.capturedSessionFactory, instanceOf( NetworkSessionFactory.class ) ); + } + + @Test + public void usesLeakLoggingSessionFactoryWhenConfigured() + { + Config config = Config.build().withLeakedSessionsLogging().toConfig(); + SessionFactoryCapturingDriverFactory factory = new SessionFactoryCapturingDriverFactory(); + + factory.newInstance( uri, dummyAuthToken(), dummyRoutingSettings(), config ); + + assertThat( factory.capturedSessionFactory, instanceOf( LeakLoggingNetworkSessionFactory.class ) ); + } + private static AuthToken dummyAuthToken() { return AuthTokens.basic( "neo4j", "neo4j" ); @@ -139,4 +161,25 @@ ConnectionPool createConnectionPool( AuthToken authToken, SecurityPlan securityP return connectionPool; } } + + private static class SessionFactoryCapturingDriverFactory extends DriverFactory + { + SessionFactory capturedSessionFactory; + + @Override + DirectDriver createDirectDriver( BoltServerAddress address, ConnectionPool connectionPool, Config config, + SecurityPlan securityPlan, SessionFactory sessionFactory ) + { + capturedSessionFactory = sessionFactory; + return null; + } + + @Override + RoutingDriver createRoutingDriver( BoltServerAddress address, ConnectionPool connectionPool, Config config, + RoutingSettings routingSettings, SecurityPlan securityPlan, SessionFactory sessionFactory ) + { + capturedSessionFactory = sessionFactory; + return null; + } + } }