Skip to content

Removed finalize from NetworkSession #285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,18 @@ 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;
}

@Override
protected Session newSessionWithMode( AccessMode mode )
{
return new NetworkSession( connections.acquire( address ) );
return sessionFactory.newInstance( connections.acquire( address ) );
}

@Override
Expand Down
34 changes: 24 additions & 10 deletions driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
{
Expand All @@ -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 ) );
}
Expand All @@ -89,21 +94,21 @@ private Driver createDriver( BoltServerAddress address, String scheme, Connectio
* <b>This method is package-private only for testing</b>
*/
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() );
}

/**
* Creates new {@link RoutingDriver}.
* <p>
* <b>This method is package-private only for testing</b>
*/
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() );
}

/**
Expand All @@ -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 ( config.logLeakedSessions() )
{
return new LeakLoggingNetworkSessionFactory( config.logging() );
}
return new NetworkSessionFactory();
}

private static SecurityPlan createSecurityPlan( BoltServerAddress address, Config config )
{
try
Expand Down
Original file line number Diff line number Diff line change
@@ -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" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error? or info? or warn?
These three are all printed out by default in command line logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leak is basically an error, that is why I use log.error here.

"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();
}
}
Original file line number Diff line number Diff line change
@@ -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.neo4j.driver.internal.spi.Connection;
import org.neo4j.driver.v1.Logger;
import org.neo4j.driver.v1.Logging;
import org.neo4j.driver.v1.Session;

class LeakLoggingNetworkSessionFactory implements SessionFactory
{
private static final String LOGGER_NAME = "sessionLeak";

private final Logger logger;

LeakLoggingNetworkSessionFactory( Logging logging )
{
this.logger = logging.getLog( LOGGER_NAME );
}

@Override
public Session newInstance( Connection connection )
{
return new LeakLoggingNetworkSession( connection, logger );
}
}
15 changes: 2 additions & 13 deletions driver/src/main/java/org/neo4j/driver/internal/NetworkSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -126,6 +126,7 @@ public static StatementResult run( Connection connection, Statement statement )
return cursor;
}

@Override
public synchronized void reset()
{
ensureSessionIsOpen();
Expand Down Expand Up @@ -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() )
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

class NetworkSessionFactory implements SessionFactory
{
@Override
public Session newInstance( Connection connection )
{
return new NetworkSession( connection );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,19 @@ 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 );
}

@Override
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 );
}

Expand Down
27 changes: 27 additions & 0 deletions driver/src/main/java/org/neo4j/driver/internal/SessionFactory.java
Original file line number Diff line number Diff line change
@@ -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;

interface SessionFactory
{
Session newInstance( Connection connection );
}
Loading