Proposal (fine to reject): opt-in safe close — Env.close(Duration) drains in-flight txns - #289
Open
bernardladenthin wants to merge 2 commits into
Open
Conversation
Two no-cost hardening changes for the close-during-read hazard that surfaces as a native SIGSEGV in mdb_txn_renew0 (see lmdbjava#253): C) Env.closed is now volatile. close() may run on a different thread than the readers calling checkNotClosed(); as a plain field there was no happens-before between the write and those reads, so a reader could indefinitely observe a stale false (the JIT may even hoist the check out of a hot loop) and proceed into a native call on a freed env. volatile does not make close() atomic w.r.t. an in-flight txnRead()/txnWrite() -- the check-then-mdb_txn_begin window remains -- but it removes the pure visibility bug and turns more of those races into a clean AlreadyClosedException instead of a JVM crash. Cost is a single volatile read on paths that already read the field. D) Env.close() now documents the lifecycle contract that LMDB's C API imposes but lmdbjava never surfaced: all txns/cursors/dbis must be closed and no other thread may be touching the env (or a handle derived from it) during close(), and violating this is undefined behaviour that crashes the JVM (SIGSEGV / EXCEPTION_ACCESS_VIOLATION 0xC0000005), not a Java exception. txnRead()/ txnWrite() gain a cross-reference and an @throws AlreadyClosedException. No behavioural change to the default hot path; consistent with the package policy that LmdbJava provides no concurrency guarantees. A follow-up branch proposes an opt-in safe close (drain in-flight readers) on top of this. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01GdiZ3ABYsVHXBRNCkEizpE
Follow-up to the volatile/docs change, proposing the real prevention for the close-during-read crash (lmdbjava#253) as an OPT-IN feature so the default hot path keeps its zero-overhead, no-concurrency-guarantees contract. New Builder.setSafeClose(boolean) (default false). When enabled, the Env tracks its live transactions and exposes Env.close(Duration): * On entry it sets a volatile `closing` flag so new txnRead()/txnWrite() throw AlreadyClosedException, then waits up to the timeout for every live txn to be closed before calling the native mdb_env_close. The map is therefore never unmapped while a read is in flight -> no SIGSEGV / mdb_txn_renew0 crash. * If txns remain open after the timeout it throws CloseTimeoutException and does NOT unmap (a leaked/stuck txn is a caller bug; forcing the unmap would reintroduce the crash). Idempotent; returns immediately if already closed. Correctness rests on a two-flag handshake: a reader increments the live count (AtomicInteger, full fence) before reading `closing`; close() writes `closing` before reading the count. With both volatile/atomic, if the drain observes zero live txns any concurrent reader observes `closing` and backs out before starting a native transaction. Registration happens in the Env txn factories before mdb_txn_begin (with decrement on begin failure); Txn.close() decrements once. Zero cost when disabled: liveTxns is null and untouched, and every tracking branch is guarded by the final safeClose flag; no change to the no-arg close(). Tests: EnvSafeCloseTest (6, all green) covers the flag, the IllegalState guard, timeout-without-close, block-until-drain + post-close rejection, idempotency, and a concurrent close-during-read stress test that is safe precisely because of this feature. Existing Env/Txn/Dbi/Cursor suites unaffected (safeClose defaults off). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01GdiZ3ABYsVHXBRNCkEizpE
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
1 participant
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.
I want to be explicit up front: it is completely reasonable to close this PR without merging. There is a legitimate design position that this kind of guard does not belong in the library at all — that lmdbjava should stay a thin, zero-overhead binding that faithfully exposes LMDB's C thread/lifecycle rules, and that serialising close against in-flight readers is the application's responsibility, not the library's. That position is consistent with the package policy ("LmdbJava DO NOT provide any concurrency guarantees … doing so would impose locking overhead on use cases that may not require it") and with @at055612's note on #253 that consumers should add "their own concurrency control around the opening of txns as an added layer of java-side protection."
I already fix this on my side in application code (a read/write lock: readers hold the read lock for the whole txn, close holds the write lock). So nothing here is needed by me — I'm offering it only in case you feel the recurring
mdb_txn_renew0crash reports (#253 and similar) are worth an in-library, opt-in convenience. If you'd rather keep guards in the application layer, just say so and close it; no hard feelings, and the docs/volatile PR (#288) stands on its own regardless.What it does (only when explicitly opted in)
New
Builder.setSafeClose(boolean)— defaultfalse— plusEnv.close(Duration):closingflag so newtxnRead()/txnWrite()throwAlreadyClosedException, then waits up to the timeout for every live txn to be closed before calling the nativemdb_env_close. The map is therefore never unmapped while a read is in flight → noSIGSEGVinmdb_txn_renew0.CloseTimeoutExceptionand does not unmap (a leaked/stuck txn is a caller bug; forcing the unmap would reintroduce the crash). Idempotent; returns immediately if already closed.Correctness rests on a two-flag handshake: a reader increments the live count (
AtomicInteger, full fence) before readingclosing;close()writesclosingbefore reading the count. With both volatile/atomic, if the drain observes zero live txns then any concurrent reader observesclosingand backs out before starting a native transaction. Registration happens in theEnvtxn factories beforemdb_txn_begin(decrement on begin failure);Txn.close()decrements once.Zero cost when disabled:
liveTxnsisnulland never touched, every tracking branch is guarded by thefinal safeCloseflag, and the no-argclose()is unchanged. Non-opting users pay nothing.Stacked on #288
This branch also contains the volatile/docs commit from #288, so this PR currently shows two commits; the first will drop out once #288 merges (or if you prefer, I can rebase this to stand alone).
Test plan
EnvSafeCloseTest(6, all green): the flag, theIllegalStateguard forclose(Duration)without opt-in, timeout-without-close, block-until-drain + post-close rejection, idempotency, and a concurrent close-during-read stress test that is safe precisely because of this feature (it would crash the JVM onmaster).Env/Txn/Dbi/Cursor/Tutorial/GCsuites unaffected (safeClose defaults off).fmt-maven-plugin:checkclean (0 non-complying).Refs #253. Builds on #288.