Removed finalize from NetworkSession #285
Conversation
| { | ||
| if ( isOpen() ) | ||
| { | ||
| log.error( "Neo4j Session object leaked, please ensure that your application" + |
There was a problem hiding this comment.
error? or info? or warn?
These three are all printed out by default in command line logging.
There was a problem hiding this comment.
Leak is basically an error, that is why I use log.error here.
| this.logger = logging.getLog( LOGGER_NAME ); | ||
| } | ||
|
|
||
| public static boolean leakLoggingEnabled() |
There was a problem hiding this comment.
wondering if we should enable it by a system variable or by a config option?
Such as
Driver driver = Graphdatabase.Driver(uri, auth, Config.build().withSessionLeakLoggingEnabled().toConfig())
There was a problem hiding this comment.
My main motivation for using system property is that users would not need to recompile their apps for leak investigation. So it would be possible to just restart it with -Dorg.neo4j.driver.logSessionLeaks=true. Do you think this makes sense?
There was a problem hiding this comment.
aha, that's good thinking. I was just thinking that they need to restart anyway, but yeah, if they do not need to change the code, that's even better
lutovich
force-pushed
the
1.1-remove-finalize
branch
from
bcd8bab to
b72e04c
Compare
|
You need to update your file header |
lutovich
force-pushed
the
1.1-remove-finalize
branch
from
b72e04c to
4517bc1
Compare
|
@lutovich Sorry, need to rebase. I am general +1 on this, while I am wondering if we want to add this special parameter as a system property or we should consider to add in Config and build a xml config reader |
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.
It was previously in system property. This commit makes it part of `Config` so we have a single place with all configuration parameters.
lutovich
force-pushed
the
1.1-remove-finalize
branch
from
4517bc1 to
b0ce429
Compare
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.logSessionLeaksto 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.