yashmayya commented on PR #19021:
URL: https://github.com/apache/pinot/pull/19021#issuecomment-5182383620
Thanks for adding the `Cleaner` — I checked and it does what it says: an
owning buffer that is never closed does get its `Arena` closed on GC, and an
explicit `close()` frees eagerly without emitting a false leak warning.
One question about the *scope* of the net. It is keyed on the reachability
of the owning `ForeignPinotBuffer`, but `view()` still returns a non-owning
buffer over `_segment.asSlice(...)` with no reference back to the owner. So if
an owning buffer becomes unreachable without being closed while a view (or a
`toDirectByteBuffer` result) is still live, the cleaner closes the arena
underneath it and subsequent reads through the view throw.
That differs from both existing implementations, where a view pins the
underlying memory:
- `UnsafePinotBuffer.view()` returns `new UnsafePinotBuffer(_memory, false,
...)`, so `MmapMemory`/`DirectMemory.finalize()` cannot run while any view is
still reachable.
- `PinotByteBuffer.view()` returns `duplicate().slice()`, and a direct
`ByteBuffer` slice pins its parent via `att`, so the parent's cleaner cannot
run either.
Quick check on JDK 25 — allocate, take a view, drop the owner reference, GC,
then read through the view:
```
foreign: owner's arena closed after owner GC = true
read through view AFTER owner GC -> IllegalStateException: Already
closed
unsafe: backing Memory closed/freed after owner GC = false
read through view AFTER owner GC = 0x11223344 (still readable)
```
So the practical effect is that a missing-close bug which used to be a quiet
native leak can now surface as an `IllegalStateException` on a read instead. Is
that intentional — i.e. is failing loudly at the read the preferred signal
here? I can see the argument for it, I would just like to confirm it is a
deliberate trade rather than a side effect, since it is a safety net that can
turn a leak into a query failure.
If it is not intentional, having `view()` hold a keep-alive reference to the
owner (or to the `ArenaCleanup`) would line the semantics up with the Unsafe
path, since the cleanable then cannot fire until every view is unreachable as
well. `toDirectByteBuffer` cannot be fixed the same way, but
`PinotDataBuffer`'s javadoc already tells callers not to use the returned
`ByteBuffer` once the receiver is released, so that one seems fine to leave as
is.
For what it is worth, the mainline path looks unaffected either way:
`SingleFileIndexDirectory` and `FilePerIndexDirectory` keep the owning buffers
in `_allocBuffers` for as long as the segment directory lives, so the views
handed to index readers always have a reachable owner.
Separately, and related — would it be worth adding a test for the net
itself? Nothing covers it today, and it is the kind of thing that regresses
silently: switching `ArenaCleanup` to a lambda that captures `this` would
disable it completely without failing any existing test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
