You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "dao-jun (via GitHub)" <gi...@apache.org> on 2026/09/25 00:44:31 UTC
[PR] [fix][managed-ledger] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
dao-jun opened a new pull request, #26709:
URL: https://github.com/apache/pulsar/pull/26709
### Motivation
The size loop in `OpReadEntry.internalReadEntriesComplete` dereferences every slot of the delivered batch:
```java
for (int i = 0; i < entriesCount; i++) {
entriesSize += returnedEntries.get(i).getLength();
}
```
A batch containing a **null slot** — a mixed range-cache read leaves its slot null when it drops an entry positioned outside the requested range — currently surfaces as a bare `NullPointerException`:
```
java.lang.NullPointerException: Cannot invoke "org.apache.bookkeeper.mledger.Entry.getLength()"
because the return value of "java.util.List.get(int)" is null
```
which is then funneled through the exception fallback (`Fallback to readEntriesFailed for exception in readEntriesComplete`). Two problems with that shape:
1. The consumer sees an opaque NPE-wrapped failure with no indication of what went wrong.
2. The fallback path never releases the delivered entries — **the whole batch's buffers leak** (the valid siblings of the null slot included).
### Modifications
- Detect a null slot up front in `internalReadEntriesComplete`, release the batch (null-safe), and fail the read with a proper `ManagedLedgerException` naming the slot index — the existing fallback forwards it as a normal read failure.
- Add a regression test delivering a batch with one null slot: it asserts the explicit failure message and a zero refcount on the valid sibling. Without the guard the test reproduces both the original NPE and the leak.
### Verifying this change
- [x] `./gradlew :managed-ledger:test --tests "org.apache.bookkeeper.mledger.impl.OpReadEntryNullSlotTest"` — passes with the guard; removing the guard turns the test red with the original NPE.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#issuecomment-5826401024
> Thanks for adding this safeguard. There is one execution path where this check is reached too late.
>
> With the in-flight read limiter enabled, `RangeEntryCacheImpl.doAsyncReadEntriesWithAcquiredPermits` wraps the callback. Its `readEntriesComplete` method iterates every entry and calls `((EntryImpl) entry).onDeallocate(...)` before invoking the original `OpReadEntry` callback.
>
> Therefore, if the assembled result is `[validEntry, null]`, the wrapper throws an NPE first and `OpReadEntry.internalReadEntriesComplete` is never called. The valid entries and the acquired limiter permit can remain retained, and the read callback is not completed by this path.
>
> The existing regression test covers the direct `OpReadEntry` behavior. An additional case going through `RangeEntryCacheImpl` with the in-flight limiter enabled would help cover this callback path as well.
Good point, I'll check it
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#discussion_r4389153695
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -134,12 +134,24 @@ public static OpReadEntry create(ManagedCursorImpl cursor, Position readPosition
return op;
}
- private void internalReadEntriesComplete(List<Entry> returnedEntries) {
+ private void internalReadEntriesComplete(List<Entry> returnedEntries) throws ManagedLedgerException {
if (returnedEntries.isEmpty()) {
log.warn().attr("op", this).log("Read no entries unexpectedly");
checkReadCompletion();
return;
}
+ // The size loop below dereferences every slot. A null slot (e.g. a mixed range-cache
+ // read whose storage leg answers short of the missing range leaves the slot unfilled)
Review Comment:
**[SUGGESTION] the null slot is rejected downstream; consider handling it where the mixed read is assembled**
The guard itself is correct (all siblings are released, one failure is delivered). My question is about the source. The tests produce the null slot by making `readUnconfirmedAsync` return a short result for the missing range, but as far as I can see a real BookKeeper read either returns every requested entry or fails the future, and the batch-read path in `ReadEntryUtils` keeps reading until the range is complete. Could you describe in the PR description the production scenario that leaves a slot null?
If a short result is possible, the mixed-read assembly in [`RangeEntryCacheImpl.java:528-548`](https://github.com/apache/pulsar/blob/06e12f040a540e401ec95e9016d5c7eea52d257b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/RangeEntryCacheImpl.java#L528-L548) already has a recovery path for a failed storage read (release what was collected and re-read the whole range through `pendingReadsManager`). Treating "the missing range came back short" the same way there would let the reader recover instead of surfacing a read failure to the consumer, and the downstream guard would remain a safety net. Non-blocking; fine to keep as is if you consider the failure the right outcome.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#issuecomment-5852046291
Thanks for adding the limiter coverage. One callback path is still not covered: when read timeout is enabled and the timeout wins before a sparse result arrives, `ManagedLedgerImpl.ReadEntryCallbackWrapper` handles the late success before `OpReadEntry`. Its late-result cleanup uses `returnedEntries.forEach(Entry::release)`, which stops at a null slot; entries after the null remain retained, and their deallocation hooks cannot return the limiter handle.
The new limiter test calls `RangeEntryCacheImpl` directly with a raw callback, so it bypasses both the timeout wrapper and the `OpReadEntry` integration. Could we add an end-to-end case covering `RangeEntryCacheImpl -> limiter wrapper -> ReadEntryCallbackWrapper -> OpReadEntry`, let the timeout win, then deliver `[valid, null, valid]`, and assert that both valid entries are released and the limiter capacity is fully restored?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#issuecomment-5833411777
Good catch — verified: with the limiter enabled the wrapper's `readEntriesComplete` dereferences every slot before the original callback runs, so a `[validEntry, null]` batch NPEs there first, and neither the callback completion nor the permit release ever happens.
Pushed a2f3a9dae0. The wrapper now treats a null slot as a share of the permits with no entry that will ever return it: `releasePermits` runs immediately for that slot, and the batch is still delivered to the original callback, where the `OpReadEntry` null-slot guard fails the read explicitly and releases the valid entries (their hooks then return the remaining permits).
The new regression test drives a mixed read through `RangeEntryCacheImpl` with the in-flight limiter enabled — the storage leg returns a short result so the assembled batch carries a null — and asserts both the delivery and the full permit return (it times out without the wrapper fix).
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#issuecomment-6442101372
Verified — the path is real, and it's now covered.
With the limiter wrapper fixed, a sparse batch does reach `readReplayRange`'s callback, whose loop called `entry.getEntryId()` on every slot: a null slot threw a bare NPE that left the wanted positions without any completion (the aggregate replay callback never resolves — the replay stays pending) and leaked the delivered entries together with their share of the in-flight read permits.
Pushed 06e12f040a. The callback now skips a null slot: the wanted position it stands for stays in `wantedEntries` and fails through the existing "Entry not found in the replay read" fallback, exactly as any entry the read didn't return (with a warn naming the ledger and range). The delivered wanted entries and the in-between entries keep their normal handling, so their deallocation hooks return the permits.
The new regression test drives a replay run end-to-end through `cursor.asyncReplayEntries` with the in-flight reads limiter enabled: the cache holds part of the run's range and the mixed read's storage leg answers the missing range [3..5] short with entries 3 and 4. It asserts the explicit failure of the missing wanted position and the full return of the limiter capacity — without the fix the replay hangs and the test times out.
The branch also merges master, where the late-completion cleanup of `ReadEntryCallbackWrapper` had evolved into `releaseUnread`; it now delegates to the null-safe `EntryImpl.releaseDiscarded` instead of duplicating it, and the PR description lists every leg of the fix.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on code in PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#discussion_r4391335096
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -134,12 +134,24 @@ public static OpReadEntry create(ManagedCursorImpl cursor, Position readPosition
return op;
}
- private void internalReadEntriesComplete(List<Entry> returnedEntries) {
+ private void internalReadEntriesComplete(List<Entry> returnedEntries) throws ManagedLedgerException {
if (returnedEntries.isEmpty()) {
log.warn().attr("op", this).log("Read no entries unexpectedly");
checkReadCompletion();
return;
}
+ // The size loop below dereferences every slot. A null slot (e.g. a mixed range-cache
+ // read whose storage leg answers short of the missing range leaves the slot unfilled)
Review Comment:
You're right about the current state of master — I traced the same paths: a plain BookKeeper read completes only when every requested entry succeeded (`PendingReadOp` counts each entry down), `ReadEntryUtils` keeps issuing batch-read requests until the range is complete, and the jcloud offload read handles (V1 and V2) read until the expected count or fail. With well-behaved storage, no in-tree leg produces a null slot today.
What made me keep the guard is that this completeness is an implicit contract that nothing enforces, while the assembly side is structurally ready to violate it: `doAsyncReadEntriesByPosition` pre-sizes the result list by the requested range and leaves a hole whenever a leg answers short. A few places where the contract is thinner than it looks:
- the BookKeeper client itself allows a batched read to return fewer entries than requested — a size-limited stop, or the verified prefix when a later entry fails its digest check (`BatchedReadOp`: "batch reads are allowed to return fewer than maxCount entries"). The continuation merge in `ReadEntryUtils` is the only layer restoring completeness, and its own comments already contemplate handing over "a read that returned no entries";
- `ReadHandle` implementations are not uniform about it: even Pulsar's own `PulsarMockReadHandle.readAsync` returns a short result instead of failing when the requested range goes past the entries it holds — which is exactly how the tests in this PR drive the scenario, and a reminder that any custom or offload-driver handle can do the same;
- and the layers around the assembly all assumed non-null slots, so the pre-fix failure mode when the contract does break is worse than an opaque NPE: with the in-flight reads limiter enabled, the wrapper dereferences the slot before the original callback runs, its remaining count never reaches zero and the limiter handle leaks permanently — an accumulating availability problem, not just a buffer leak; the late-timeout cleanup stops at the slot; and a replay range read hung without completing its wanted positions.
I've added this to the PR description ("Where a null slot comes from"). I kept the explicit failure as the outcome — a sparse batch means the read did not get what it asked for, and surfacing the failure keeps the contract break visible. Treating the short answer at the source — release what was collected and re-read the whole range, like the failed-read recovery — is a reasonable next step; it can be pursued separately if there's interest.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#issuecomment-5852025776
I took a wider look at the callback/ownership chain. The null-slot accounting added in `RangeEntryCacheImpl` is locally balanced for the normal completion path, but a sparse batch still has no single cleanup owner, and one path bypasses the new `OpReadEntry` guard entirely.
With read timeouts enabled, completion unwinds as `RangeEntryCacheImpl` limiter wrapper -> `ManagedLedgerImpl.ReadEntryCallbackWrapper` -> `OpReadEntry`. If the timeout has already won, `ReadEntryCallbackWrapper.readEntriesComplete` executes `returnedEntries.forEach(Entry::release)`. For a late result such as `[entry0, null, entry2]`, cleanup stops at the null: `entry2` remains retained, its `onDeallocate` hook never fires, and the aggregate limiter handle remains acquired. The `OpReadEntry` null guard never sees this batch.
More generally, I think `RangeEntryCacheImpl` should be the terminal invariant boundary for mixed-read assembly instead of successfully forwarding a sparse `List<Entry>` through the limiter and timeout layers. Each missing-read future has an exact requested subrange, so the merge can validate that entries are contiguous, belong to that subrange, and do not target an already occupied slot. On extra/duplicate/occupied entries or a final hole, it should release every owned entry and invoke `readEntriesFailed` (or perform one explicit full-range retry), rather than deliver a success containing null. The `OpReadEntry` check can remain as defense in depth. The late-result cleanup should also be null-safe/best-effort so one malformed slot cannot prevent the remaining entries from being released.
The new limiter test calls the cache directly with a raw callback, so it bypasses both the timeout wrapper and the `OpReadEntry` integration. It also mocks `readUnconfirmedAsync(2, 4)` as successfully returning only `[2, 3]`; a normal BookKeeper range read does not have that successful short-result contract. An end-to-end cursor test where timeout wins and a malformed/sparse result arrives late would cover the missing ownership path.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#discussion_r4230602477
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -122,12 +122,28 @@ public static OpReadEntry create(ManagedCursorImpl cursor, Position readPosition
return op;
}
- private void internalReadEntriesComplete(List<Entry> returnedEntries) {
+ private void internalReadEntriesComplete(List<Entry> returnedEntries) throws ManagedLedgerException {
if (returnedEntries.isEmpty()) {
log.warn().attr("op", this).log("Read no entries unexpectedly");
checkReadCompletion();
return;
}
+ // The size loop below dereferences every slot. A null slot (e.g. a mixed range-cache
+ // read leaves its slot null when it drops an out-of-range entry) would throw a bare NPE
+ // whose fallback path leaks the whole batch's buffers. Fail the read explicitly instead:
+ // release the batch and surface a proper read failure.
+ for (int i = 0; i < returnedEntries.size(); i++) {
+ if (returnedEntries.get(i) == null) {
+ log.warn().attr("op", this).attr("slot", i)
+ .log("Delivered entry batch contains a null slot");
+ returnedEntries.forEach(entry -> {
+ if (entry != null) {
+ entry.release();
+ }
+ });
+ throw new ManagedLedgerException("Delivered entry batch contains a null slot at index " + i);
Review Comment:
One more thing I noticed here: `EntryImpl.release()` also calls `readCountHandler.markRead()` by default.
In this case, we're discarding the non-null entries because the batch is malformed. They haven't been successfully delivered, and the read position hasn't advanced past this batch. Decrementing their shared `expectedReadCount` could make the cached entries eligible for eviction too early.
Could we set `setDecreaseReadCountOnRelease(false)` before releasing these entries? That should still allow their `onDeallocate` hooks to return the limiter permits.
A regression test with a positive expected-read count would be helpful here too. This looks similar to the dropped-entry accounting issue in #26708.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ml] Fail the read explicitly when a delivered batch contains a null slot [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on PR #26709:
URL: https://github.com/apache/pulsar/pull/26709#issuecomment-6397979161
@dao-jun @void-ptr974 @lhotari
One more path worth covering here is `ManagedCursorImpl.readReplayRange`, introduced by #27006.
It iterates over the range result and calls `entry.getEntryId()` without checking for null. As the mixed cache/storage test in this PR shows, a short storage read can leave null slots in the assembled result.
Fixing the inflight limiter wrapper would allow that result to reach the replay callback, but the callback could still throw an NPE, leaving some wanted positions without a completion and potentially keeping the replay pending.
Could we cover this path as well, ideally with a focused replay test that checks failure completion and that the non-null entries and permits are properly released?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org