Fix #215: correct NIO buffer for Netty ByteBuf reads (view + copy, pick one) - #290
Open
bernardladenthin wants to merge 3 commits into
Open
Fix #215: correct NIO buffer for Netty ByteBuf reads (view + copy, pick one)#290bernardladenthin wants to merge 3 commits into
bernardladenthin wants to merge 3 commits into
Conversation
…le data A value ByteBuf returned via ByteBufProxy.PROXY_NETTY reads correctly through the ByteBuf's own accessors, but ByteBuf.nioBuffer() does not reflect the stored data (it views Netty's separate, chunk-shared backing buffer, which the zero-copy read path never repoints). This test pins the current behaviour; the following commits add helpers to obtain a correct NIO buffer. Refs lmdbjava#215. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01GdiZ3ABYsVHXBRNCkEizpE
Adds a static helper that materialises a read-only NIO ByteBuffer aliasing the LMDB memory referenced by a PROXY_NETTY value ByteBuf, using the same Unsafe address/capacity technique ByteBufferProxy already uses for its own zero-copy NIO buffers. This lets Netty users hand a correct NIO buffer to NIO-based consumers (compression, hashing, ...) instead of getting the zeros that ByteBuf.nioBuffer() returns. The java.nio.Buffer field offsets are resolved lazily in a holder so a JVM lacking --add-opens java.base/java.nio only fails if nioBufferView is actually called, rather than breaking PROXY_NETTY initialisation for everyone. The returned buffer aliases LMDB-owned, read-only memory and is valid only while the owning read Txn is open — documented prominently, as misuse is a JVM crash. Refs lmdbjava#215. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01GdiZ3ABYsVHXBRNCkEizpE
…ative Adds a second static helper as an alternative to nioBufferView: instead of aliasing LMDB memory, it copies the readable bytes out via the ByteBuf's own (correct) accessor into a fresh direct NIO buffer. This uses no Unsafe/reflection, needs no --add-opens, and — unlike the view — remains valid after the owning transaction (or the Env) is closed. The two helpers are deliberately offered together so the maintainers can choose: keep the zero-copy view, the safe copy, or both. Dropping this commit removes the copy variant; dropping the previous commit removes the view. Recommendation: nioBufferCopy as the safe default, nioBufferView for zero-copy hot paths. Refs lmdbjava#215. 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
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.
Fixes #215, where
ByteBuf.nioBuffer()on a value returned viaByteBufProxy.PROXY_NETTYyields zeros.Root cause (confirming @2018ik)
For zero copy, a read-returned
ByteBufis repointed at LMDB's mmap by overwriting itsmemoryAddress(ByteBufProxy.out). TheByteBuf's own accessors (getBytes, …) then read the LMDB data correctly, butByteBuf.nioBuffer()derives its NIO buffer from Netty's separate, chunk-shared backing buffer, which was never repointed — so it reads zeros. That chunk buffer cannot simply be repointed (as @2018ik suggested): it is shared by every pooled buffer carved from the same chunk, so overwriting its address would corrupt sibling buffers.This is a "pick one" PR
Rather than guess, I've implemented both reasonable fixes as separate commits so you can choose and drop the other:
ByteBufProxy.nioBufferViewByteBufProxy.nioBufferCopyUnsafeaddress/capacity trickByteBufferProxyalready uses)ByteBuf.getBytesinto a fresh direct bufferEnvclose--add-opens java.base/java.nioCommits:
test(#215)— repro test pinning the current (buggy) behaviour.feat(#215)—nioBufferView(zero-copy) + test.feat(#215)—nioBufferCopy(safe copy) + tests.→ Keep both, or drop commit 3 (view only) or commit 2 (copy only). I'll remove the unwanted variant (and its one cross-reference) in a follow-up.
Recommendation
My personal preference is to keep both
nioBufferViewandnioBufferCopyand document the trade-off clearly (the table above), so callers can pick per use case: zero-copy when the buffer is consumed within the transaction, or the safe copy when the bytes must outlive it. To me that is the cleanest outcome — the two genuinely serve different needs, and offering both (with the lifetime caveat spelled out) is more useful than forcing one.That said, this is entirely your call. If you'd rather minimise API surface, I'm happy to keep just one — I'd suggest
nioBufferCopyas the safe default — or to land only the repro test + docs and add no new API at all.Notes / testing
ByteBufNioBufferTest(4 tests, all green): view reflects data, copy reflects data, copy survives txn+env close (the key discriminator), and the rawnioBuffer()bug is documented.ByteBufProxyTestunaffected;fmt-maven-plugin:checkclean.