You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Depending on how tests are run I am seeing an occasional segmentation fault. It happens in tests due to execution of subsequent tests, i.e. running tests on their own does not cause a segfault. I have tracked the issue down to tests that leave transactions unclosed, e.g. tests that create an env, open a transaction and then close the env without closing the transaction first.
The LmdbJava code deliberately prevents transactions from being used or closed after envs are closed as any use is expected to cause a segfault. The tests are designed to ensure that LmdbJava prevents this and tests the behaviour. However these tests leave LMDB in a state that will then cause a random segfault in some future scenario. I have a test case that demonstrates this happening quite reliably.
If I add code to force the env to track all open transactions and ensure they are closed before the env closes then there is no problem, however this would introduce some overhead keeping track of transactions in a way that is thread safe.
I think that the issue might be something to do with LMDB keeping track of transactions per thread as the issue is linked to the use of multiple threads. This is why it fails in some test execution scenarios that use multiple threads and not others. Note that it still fails with MDB_NOTLS used.
I'm not sure what the answer is. We obviously want to test that LmdbJava guards against incorrect usage but a situation that leads to intermittent segfault at some random later point is bad news. It might be something that can be fixed by the maintainer of LMDB if the problem is better understood at a deeper level.
Only a single thread may call this function. All transactions, databases, and cursors must already be closed before calling this function. Attempts to use any such handles after calling this function will cause a SIGSEGV. The environment handle will be freed and must not be used again after this call.
While it would be nice for LmdbJava to track all the open txns and prevent the Env from closing while any are open, I think this would be just too much overhead on the opening/closing of txns which is a very hot path.
I think the javadoc on the Env#close method should be improved to make it very clear that closing the env when any dbs/txns/cursors are open may result in a SIGSEGV. The responsibility is then on the caller to adhere to that contract.
Unless @benalexau has any other ideas, I also think the problem tests that try to assert that txns can't be closed after the env is closed ought to be removed if we have no way to clean up the invalid state that they have created. They are trying to test something that the contract is stating not to do.
We can use an AtomicInteger reference counting pattern:
Increment on Txn constructor
Decrement on Txn.close()
Block on Env.close() if count is non-zero
Wrap the reference counting behind our existing Env.SHOULD_CHECK mechanism so users can set lmdbjava.disable.checks to opt out. Since it's a static, the JVM optimizes it away anyway.
We should benchmark to confirm, but I'd be surprised if AtomicInteger operations (especially uncontended) measurably impact latency compared to FFI and I/O overhead for the transaction itself.
I'll create a PR that uses AtomicInteger to count open Txns that will throw an exception if count > 0 on env close. I will probably have to alter the tests so we aren't breaking the contract deliberately in tests as it is often causing other tests to fail. I'll also add the same open txn count protection to parent Txns to ensure all child Txns are closed before the parent as this also causes the same sort of problems. I'll make sure all of the new behaviour is within Env.SHOULD_CHECK.
I'll wait until AT has his big PR merged as I don't want to add complexity to that change.
I wonder if we can use LongAdder instead of AtomicInteger. LongAdder is better in high concurrency situations as it holds the total in multiple 'buckets' to reduce contention. We only care about the total sum() in Env.close.
public long sum()
Returns the current sum. The returned value is NOT an atomic snapshot; invocation in the absence of concurrent updates returns an accurate result, but concurrent updates that occur while the sum is being calculated might not be incorporated.
However, the private boolean closed; field and checkNotClosed() should ensure that no Txn is opened once close() has been called. I am pretty sure closed ought to be volatile or an AtomicBoolean though.
Agreed, closed should be volatile. Re LongAdder, I don't think we are likely to have high contention situations. The extra complexity of accumulating the cells seems likely to exceed the simple CAS operation underpinning AtomicInteger. Plus we have SHOULD_CHECK, so if performance really became a concern the user can switch the entire mechanism off.
I don't think we are likely to have high contention situations.
I'm not sure that is true. One of our uses of LJ is for lookups of reference data to decorate event streams. Many threads (some servers have 100+ cores) will each be processing event streams, and each process will be doing many 000s of lookups (open read txn, dbi.get, close txn) on the same env. There will almost certainly be contention on the txn reference counter.
These benchmarks would suggest that you only need 2 threads in play for LongAdder to show a significant benefit and beyond 2 it is a clear winner. It does however confirm that for a single thread there is a cost. Ultimately, we need to benchmark many threads all doing open-get-close for LongAdder vs AtomicInteger to see how they perform in a more representative scenario. http://blog.palominolabs.com/2014/02/10/java-8-performance-improvements-longadder-vs-atomiclong/
One option would be for the Env builder to have something like
Env.create()
...
.singelThreaded()
.open(...)
so that the Env knows if it needs to be thread safe or not. Some uses of LJ may be purely single threaded and thus don't need/want the added overhead of concurrency protection. The Env can then be constructed to suit that setting, e.g. an int based ref counter vs a LongAdder based one.
People will use LJ in all sorts of different ways so if the Env/Dbi has more information about how they are being used (e.g. single/multi threaded, fixed/variable length keys), they can then include minor optimisations to cater to the use case.
In my usage of lmdbjava, I open an environment at the beginning, my system runs, and then the environment is closed as the application is shutdown. Given the documentation, if I were opening and closing the environment repeatedly, I would expect to manage the transactions and ensure all are closed before trying to close the environment. I'm usually not keen on fixing tests to make them work, but in this case, isn't it simply the case that the tests are doing the wrong thing? We're running lmdbjava for its excellent performance, and, as has been discussed previously, some of that performance comes with 'foot guns' in terms of currently unsafe methods and an ability to create segfaults not possible without the use of native code.
It sounds as if the suggested approach of counting transactions will stop the errors during test running, but I am unaware of any reports of this kind of issue from users of lmdbjava in live usage, probably because those usages close all transactions. Most of my transactions are extremely short, and any server code would not shutdown until all threads, and therefore all transactions have ended.
I think in most real-world usage, surely closing the environment would occur only a) at application shutdown, or b) as part of a lifecycle in which all threads using that environment will have ended because one has built a proper shutdown process?
While the counting of transactions will likely not add significant overhead, it does add more complexity, and I think I'd be in favour of simply fixing the tests to use more appropriate behaviour [and therefore be better examples for others to follow] and improve the documentation.
I'm not sure that is true. One of our uses of LJ is for lookups of reference data to decorate event streams. Many threads (some servers have 100+ cores) will each be processing event streams, and each process will be doing many 000s of lookups (open read txn, dbi.get, close txn) on the same env. There will almost certainly be contention on the txn reference counter.
Why not use a long-running Txn for each thread? It doesn't sound like there are many mutations (being reference data) and that's when you'd generally want to be more careful with long-running transactions. A heavy, read-dominated workload would usually benefit from a long-running Txn due to fewer allocations, GC pressure, and FFI crosses associated with managing the Txn handle.
Still, I acknowledge your broader point that sometimes there may be lots of threads running on many cores.
These benchmarks would suggest that you only need 2 threads in play for LongAdder to show a significant benefit
I note that benchmark was run under Java 8 circa 2014. We should probably re-run it on modern JVMs and compare AtomicInteger, AtomicLong, and LongAdder. Would anyone be able to run this?
.... an ability to create segfaults not possible without the use of native code
Like most C libraries, LMDB documents many requirements in order to avoid SIGSEGV. Quoting our package-info.java:
The classes in LmdbJava DO NOT provide any concurrency guarantees. Instead you MUST observe
LMDB's specific thread rules (eg do not share transactions between threads). LmdbJava does not
shield you from these requirements, as doing so would impose locking overhead on use cases that
may not require it or have already carefully implemented application threading (as most low
latency applications do to optimize the memory hierarchy, core pinning etc).
That said, if we can add safety checks that are zero-cost when SHOULD_CHECK is disabled, and the code delta is small, it seems worth doing to help users avoid fatal errors where we can.
I think I'd be in favour of simply fixing the tests to use more appropriate behaviour [and therefore be better examples for others to follow] and improve the documentation.
Agreed. Let's fix any tests that don't close transactions properly or document why they deliberately don't.
We should probably re-run it on modern JVMs and compare AtomicInteger, AtomicLong, and LongAdder.
That benchmark repo was so out of date I ended up just crafting a quick jmh class to test AI, AL, LA and a primitive int (for reference and accepting that it is pointless where threads >1). This is using Java 21 temurin on a 12core 24thread cpu.
In my usage of lmdbjava, I open an environment at the beginning, my system runs, and then the environment is closed as the application is shutdown. Given the documentation, if I were opening and closing the environment repeatedly, I would expect to manage the transactions and ensure all are closed before trying to close the environment. I'm usually not keen on fixing tests to make them work, but in this case, isn't it simply the case that the tests are doing the wrong thing? We're running lmdbjava for its excellent performance, and, as has been discussed previously, some of that performance comes with 'foot guns' in terms of currently unsafe methods and an ability to create segfaults not possible without the use of native code.
It sounds as if the suggested approach of counting transactions will stop the errors during test running, but I am unaware of any reports of this kind of issue from users of lmdbjava in live usage, probably because those usages close all transactions. Most of my transactions are extremely short, and any server code would not shutdown until all threads, and therefore all transactions have ended.
I think in most real-world usage, surely closing the environment would occur only a) at application shutdown, or b) as part of a lifecycle in which all threads using that environment will have ended because one has built a proper shutdown process?
While the counting of transactions will likely not add significant overhead, it does add more complexity, and I think I'd be in favour of simply fixing the tests to use more appropriate behaviour [and therefore be better examples for others to follow] and improve the documentation.
I actually have a very similar setup:
Open the Envs (I've 32) on startup
Open and use a tx-write every 20s for each Env
Open and use a tx-read every 40s for each Env
@benalexau long-living tx-reads are only good if the writes are really rare.
Moreover, at least to my experience with this so far, as soon as a tx-write is open and a tx-read starts then the space in the Env file starts to grow quite a lot due to the copy-on-write mechanism LMDB has, ending up potentially in some LMDB error like MapSizeExeeded or so.
For example, I'm not able to provide a potential API exposing the data I've in LMDB exactly for this behavior.
It sounds as if the suggested approach of counting transactions will stop the errors during test running, but I am unaware of any reports of this kind of issue from users of lmdbjava in live usage, probably because those usages close all transactions.
@wardle I kind of had similar issues like the one presented in here in the past and I still have randomly from time to time, but it's difficult for me to say it's because of some race condition on closing the resources or because of something else.
Activity
[-]SegFault running tests[/-][+]SIGSEGV running tests[/+]#279 Test to demonstrate segfault
stroomdev66 commentedon Nov 12, 2025
See branch
gh-279_segfaultandTestSegfaultto demoat055612 commentedon Nov 12, 2025
From the lmdb docs for
mdb_env_close(http://www.lmdb.tech/doc/group__mdb.html#ga4366c43ada8874588b6a62fbda2d1e95):While it would be nice for LmdbJava to track all the open txns and prevent the Env from closing while any are open, I think this would be just too much overhead on the opening/closing of txns which is a very hot path.
I think the javadoc on the
Env#closemethod should be improved to make it very clear that closing the env when any dbs/txns/cursors are open may result in a SIGSEGV. The responsibility is then on the caller to adhere to that contract.Unless @benalexau has any other ideas, I also think the problem tests that try to assert that txns can't be closed after the env is closed ought to be removed if we have no way to clean up the invalid state that they have created. They are trying to test something that the contract is stating not to do.
bp-alex commentedon Nov 12, 2025
I agree we should improve the documentation.
We can use an
AtomicIntegerreference counting pattern:TxnconstructorTxn.close()Env.close()if count is non-zeroWrap the reference counting behind our existing
Env.SHOULD_CHECKmechanism so users can setlmdbjava.disable.checksto opt out. Since it's a static, the JVM optimizes it away anyway.We should benchmark to confirm, but I'd be surprised if
AtomicIntegeroperations (especially uncontended) measurably impact latency compared to FFI and I/O overhead for the transaction itself.stroomdev66 commentedon Nov 13, 2025
I'll create a PR that uses
AtomicIntegerto count open Txns that will throw an exception if count > 0 on env close. I will probably have to alter the tests so we aren't breaking the contract deliberately in tests as it is often causing other tests to fail. I'll also add the same open txn count protection to parent Txns to ensure all child Txns are closed before the parent as this also causes the same sort of problems. I'll make sure all of the new behaviour is withinEnv.SHOULD_CHECK.I'll wait until AT has his big PR merged as I don't want to add complexity to that change.
at055612 commentedon Nov 13, 2025
I wonder if we can use LongAdder instead of AtomicInteger.
LongAdderis better in high concurrency situations as it holds the total in multiple 'buckets' to reduce contention. We only care about the totalsum()in Env.close.However, the
private boolean closed;field andcheckNotClosed()should ensure that noTxnis opened onceclose()has been called. I am pretty sureclosedought to bevolatileor anAtomicBooleanthough.bp-alex commentedon Nov 13, 2025
Agreed,
closedshould bevolatile. ReLongAdder, I don't think we are likely to have high contention situations. The extra complexity of accumulating the cells seems likely to exceed the simple CAS operation underpinningAtomicInteger. Plus we haveSHOULD_CHECK, so if performance really became a concern the user can switch the entire mechanism off.at055612 commentedon Nov 14, 2025
I'm not sure that is true. One of our uses of LJ is for lookups of reference data to decorate event streams. Many threads (some servers have 100+ cores) will each be processing event streams, and each process will be doing many 000s of lookups (open read txn, dbi.get, close txn) on the same env. There will almost certainly be contention on the txn reference counter.
These benchmarks would suggest that you only need 2 threads in play for LongAdder to show a significant benefit and beyond 2 it is a clear winner. It does however confirm that for a single thread there is a cost. Ultimately, we need to benchmark many threads all doing open-get-close for LongAdder vs AtomicInteger to see how they perform in a more representative scenario.
http://blog.palominolabs.com/2014/02/10/java-8-performance-improvements-longadder-vs-atomiclong/
One option would be for the Env builder to have something like
so that the Env knows if it needs to be thread safe or not. Some uses of LJ may be purely single threaded and thus don't need/want the added overhead of concurrency protection. The Env can then be constructed to suit that setting, e.g. an
intbased ref counter vs a LongAdder based one.People will use LJ in all sorts of different ways so if the Env/Dbi has more information about how they are being used (e.g. single/multi threaded, fixed/variable length keys), they can then include minor optimisations to cater to the use case.
wardle commentedon Nov 14, 2025
Can I play Devil's advocate here?
In my usage of lmdbjava, I open an environment at the beginning, my system runs, and then the environment is closed as the application is shutdown. Given the documentation, if I were opening and closing the environment repeatedly, I would expect to manage the transactions and ensure all are closed before trying to close the environment. I'm usually not keen on fixing tests to make them work, but in this case, isn't it simply the case that the tests are doing the wrong thing? We're running lmdbjava for its excellent performance, and, as has been discussed previously, some of that performance comes with 'foot guns' in terms of currently unsafe methods and an ability to create segfaults not possible without the use of native code.
It sounds as if the suggested approach of counting transactions will stop the errors during test running, but I am unaware of any reports of this kind of issue from users of lmdbjava in live usage, probably because those usages close all transactions. Most of my transactions are extremely short, and any server code would not shutdown until all threads, and therefore all transactions have ended.
I think in most real-world usage, surely closing the environment would occur only a) at application shutdown, or b) as part of a lifecycle in which all threads using that environment will have ended because one has built a proper shutdown process?
While the counting of transactions will likely not add significant overhead, it does add more complexity, and I think I'd be in favour of simply fixing the tests to use more appropriate behaviour [and therefore be better examples for others to follow] and improve the documentation.
bp-alex commentedon Nov 14, 2025
Why not use a long-running
Txnfor each thread? It doesn't sound like there are many mutations (being reference data) and that's when you'd generally want to be more careful with long-running transactions. A heavy, read-dominated workload would usually benefit from a long-runningTxndue to fewer allocations, GC pressure, and FFI crosses associated with managing theTxnhandle.Still, I acknowledge your broader point that sometimes there may be lots of threads running on many cores.
I note that benchmark was run under Java 8 circa 2014. We should probably re-run it on modern JVMs and compare
AtomicInteger,AtomicLong, andLongAdder. Would anyone be able to run this?Like most C libraries, LMDB documents many requirements in order to avoid
SIGSEGV. Quoting ourpackage-info.java:That said, if we can add safety checks that are zero-cost when
SHOULD_CHECKis disabled, and the code delta is small, it seems worth doing to help users avoid fatal errors where we can.Agreed. Let's fix any tests that don't close transactions properly or document why they deliberately don't.
at055612 commentedon Nov 17, 2025
That benchmark repo was so out of date I ended up just crafting a quick jmh class to test AI, AL, LA and a primitive
int(for reference and accepting that it is pointless where threads >1). This is using Java 21 temurin on a 12core 24thread cpu.This confirms that for all but single threaded operation, LA is orders of magnitude faster than AI/AL even with only 2 threads in play.
If the Env.Builder were to have a
.singleThreaded()method then the txn ref counter could useintfor singlethreaded mode and LA for everything else.cdprete commentedon Nov 18, 2025
I actually have a very similar setup:
@benalexau long-living tx-reads are only good if the writes are really rare.
Moreover, at least to my experience with this so far, as soon as a tx-write is open and a tx-read starts then the space in the Env file starts to grow quite a lot due to the copy-on-write mechanism LMDB has, ending up potentially in some LMDB error like MapSizeExeeded or so.
For example, I'm not able to provide a potential API exposing the data I've in LMDB exactly for this behavior.
@wardle I kind of had similar issues like the one presented in here in the past and I still have randomly from time to time, but it's difficult for me to say it's because of some race condition on closing the resources or because of something else.
27 remaining items
gh-279 Fix catch in Cursor ctor
gh-279 Tidy asserts in TxnTest
gh-279 Add Env.tryClose, add more tests
gh-279 Add test for Cursor ctor exception
gh-279 Remove dead code, add test for code coverage
gh-279 Improve Env.copy tests
gh-279 Add RefCounter.run test
gh-279 Fix AbstractFlagSetTest
gh-279 Changes to AbstractFlagSetTest for codecov
gh-279 Format
gh-279 Add tests to TargetNameTest
gh-279 Change safeClose to only track RW cursors
gh-279 Add more tests, improve javadoc
gh-279 Fix codeQL suggestion
gh-279 Tweak javadoc