Gh 279 env close protection - #293
Open
at055612 wants to merge 62 commits into
Open
Conversation
Make perf test runs more consistent in terms of work done
Add more cursor tests, improve javadoc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #279
The aim of this PR is to add optional protection to prevent the Env from being closed if there are transactions or cursors open. #279 discussed some options for implementing this protection using reference counting including using a simple AtomicInteger and LongAdder. LongAdder is no-go as it is not thread safe in the way we need to use it.
This PR uses a reference counter that uses an array of AtomicIntegers for improved multi threaded performance. This is similar to what LongAdder does, but adds in concurrency controls to allow a count to happen in a thread safe way. I have various tests to check this and AI has checked over it, but would welcome anyone picking holes in it in case I have missed some race condition.
Rather than conflate this protection with the existing
SHOULD_CHECKlogic, it is enabled via these four newEnv.Buildermethods:It may be beneficial to add a new system property to turn on/off safeClose to allow it to be turned on in dev, however users of the library can create their own system property to control the value for
setSafeClose(enabled).If
safeCloseandsingleThreadedare set, a non-thread-safe reference counter is used that relies on a simpleintand no concurrency protection. This will be faster and use less memory.If
safeCloseandsingleThreadedare both un-set, the Env will use a no-op implementation of RefCounter. I would hope that the JVM will optimise these no-op method calls out to remove the tiny overhead that they add.If only
safeCloseis set, the striped thread safe reference counter implementation will be used.If only
singleThreadedis set, this has no impact on the Env, as this flag currently only impacts the choice of reference counter impl. It is possible that thesingleThreadedflag on the env could also be used in the future to control how LMDBJava implements other things when used in a strictly single threaded way.When
safeCloseis set, callingEnv.close()will result in aEnvInUseExceptionbeing thrown if there are open cursors/txns. It will not block or wait.I started this PR quite a while ago, but have changed it to adopt some of the builder changes from #289. I disagree with the
close(timeout)in #289 on the basis that it should be the responsibility of the calling code to determine when to close the env, rather than LMDBJava's.I have added a
tryClose()method which returns true if the close could be performed or false otherwise. WithsafeCloseenabled, this meanstryClose()will return false if the env is in use rather than throwing.