-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Close all resources gracefully on JVM shutdown #98
base: develop
Are you sure you want to change the base?
Conversation
public void close() { | ||
lock.lock(); | ||
try { | ||
chronicleMap.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @the123saurav, please consider this case https://github.com/OpenHFT/Chronicle-Map/blob/ea/docs/CM_Tutorial.adoc#close-chroniclemap
If you call close() too early before you have finished working with the map, this can cause your JVM to crash. Close MUST be the last thing that you do with the map.
So, although it is recommended that you close() when you have finished with the map, it is not something that you must do; it’s just something that we recommend you should do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point. But does that stop us from calling this?
It will be called from 2 places:
- During JVM shutdown: In this case we are guranteed to not get any calls.
- User initiates shutdown in embedded mode. In this case we can just add to our doc, that this should be the last thing done to KevaServer.
I am also okay to remove this from shutdown procedure.
Kindly let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @the123saurav, I think we can add that warning to our doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Can this be merged now? @the123saurav |
1a702d1
to
dcff9ea
Compare
5ca77a9
to
b1c70d3
Compare
This change closes all resources(store, aof) on JVM shutdown.
It also introduces a notion of state in server and adds check on API methods to prevent misuse/race conditions.