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/26 08:06:27 UTC
[PR] [test][ml] Make the EntryImpl poison-position regression test actually recycle [pulsar]
dao-jun opened a new pull request, #26721:
URL: https://github.com/apache/pulsar/pull/26721
### Motivation
The post-merge review of #26707 (thanks @void-ptr974) pointed out that the regression test added there does not actually exercise recycling: on Netty 4.2 the `Recycler` only pools instances for `FastThreadLocalThread`s — a plain thread receives a no-op handle, so every `get()` allocates a fresh object. The merged test ran on an ordinary test worker thread and still passes with the three `entry.position = null` resets removed.
The review also correctly noted that the comment above the reset overclaims: a `getPosition()` concurrent with `create()` can still read the old ids and publish its stale value after the reset (unsynchronized fields, no generation check), so the reset is best-effort hardening for the *sequential* poison-and-reuse case — post-release access remains a caller bug to fix at the call site.
### Modifications
- `EntryImplTest.testRecycledObjectDoesNotInheritPoisonedPosition` now runs the poison-and-reuse scenario on a `FastThreadLocalThread` and proves instance identity with `assertSame`, guarded by a bounded warm-up that fails the test if the per-thread pool never hands the same instance back (absorbs `io.netty.recycler.ratio` warm-up behavior).
- The scenario now covers all three lazy-position `create(...)` variants, including `create(LedgerEntry, int)` — the variant the managed-ledger read path actually uses.
- Reworded the comment above the reset in `EntryImpl.create(LedgerEntry, int)` to state the actual guarantee and its limits (sequential case only; concurrent post-release access is out of scope and must be fixed at the call site).
No production behavior change — the three reset lines are untouched.
### Verifying this change
- [x] `./gradlew :managed-ledger:test --tests "org.apache.bookkeeper.mledger.impl.EntryImplTest"` — 12/12 green.
- [x] Red/green verified: with the three `entry.position = null` resets temporarily removed, the new test fails (`create(LedgerEntry, int): a recycled entry must not inherit the poisoned (-1, -1) position`); restoring them is green.
This change is a trivial rework: no production logic is modified beyond a comment.
--
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] [test][ml] Make the EntryImpl poison-position regression test actually recycle [pulsar]
Posted by "utafrali (via GitHub)" <gi...@apache.org>.
utafrali commented on code in PR #26721:
URL: https://github.com/apache/pulsar/pull/26721#discussion_r4110670446
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/EntryImplTest.java:
##########
@@ -260,29 +266,100 @@ public void testCreateWithPositionThatIsImmutable() {
}
@Test
- public void testRecycledObjectDoesNotInheritPoisonedPosition() {
+ public void testRecycledObjectDoesNotInheritPoisonedPosition() throws Exception {
+ // Netty's Recycler only pools instances for FastThreadLocalThreads; a plain test worker
+ // thread receives a no-op handle, every create() would return a fresh object, and this
+ // scenario would silently degrade to asserting fresh instances (see the review on #26707).
+ // Run it on a FastThreadLocalThread and prove instance identity with assertSame.
+ runOnFastThreadLocalThread(() -> {
+ assertTrue(warmUpRecyclerUntilReused(),
+ "recycler never handed the same instance back; the scenario cannot exercise recycling");
+
+ // The managed-ledger read path wraps a BookKeeper LedgerEntry.
+ assertRecycledPoisonedPositionIsNotInherited("create(LedgerEntry, int)",
+ () -> createFromLedgerEntry(5L, 10L),
+ () -> createFromLedgerEntry(6L, 20L), 6L, 20L);
+ // byte[] variant.
+ assertRecycledPoisonedPositionIsNotInherited("byte[] variant",
+ () -> EntryImpl.create(5L, 10L, new byte[]{1, 2, 3}),
+ () -> EntryImpl.create(6L, 20L, new byte[]{4, 5, 6}), 6L, 20L);
+ // ByteBuf variant.
+ assertRecycledPoisonedPositionIsNotInherited("ByteBuf variant",
+ () -> createFromRetainedBuffer(5L, 10L),
+ () -> createFromRetainedBuffer(6L, 20L), 6L, 20L);
+ });
+ }
+
+ private static void assertRecycledPoisonedPositionIsNotInherited(String variant,
+ Supplier<EntryImpl> firstEntry,
+ Supplier<EntryImpl> secondEntry,
+ long expectedLedgerId,
+ long expectedEntryId) {
// Given a legitimate entry that is released normally
- EntryImpl first = EntryImpl.create(5L, 10L, new byte[]{1, 2, 3});
+ EntryImpl first = firstEntry.get();
first.release();
// When a getPosition() call slips in AFTER the release: deallocation nulls the lazy
// position field, so this late reader re-materializes it from the reset ids as (-1, -1)
// and leaves the poisoned value cached inside the pooled object.
first.getPosition();
- // Then the next create() (the recycler hands back the most recently released object on
- // the same thread) must not report that stale (-1, -1) position as its own — through
- // both the byte[] and the ByteBuf variants, which own the lazy field.
- EntryImpl second = EntryImpl.create(6L, 20L, new byte[]{4, 5, 6});
- assertTrue(second.getPosition().compareTo(PositionFactory.create(6L, 20L)) == 0,
- "byte[] variant: a recycled entry must not inherit the poisoned (-1, -1) position");
+ // Then the very next create() on this thread reuses that exact instance and must not
+ // report the poisoned (-1, -1) position as its own.
+ EntryImpl second = secondEntry.get();
+ assertSame(second, first, variant + ": expected the recycler to reuse the same instance");
+ assertTrue(second.getPosition().compareTo(PositionFactory.create(expectedLedgerId, expectedEntryId)) == 0,
+ variant + ": a recycled entry must not inherit the poisoned (-1, -1) position");
second.release();
+ }
+
+ private static EntryImpl createFromLedgerEntry(long ledgerId, long entryId) {
+ ByteBuf buffer = Unpooled.wrappedBuffer(new byte[]{1, 2, 3});
+ LedgerEntry ledgerEntry = LedgerEntryImpl.create(ledgerId, entryId, buffer.readableBytes(), buffer);
+ // LedgerEntryImpl.create adopts the caller's buffer reference (released again on close()),
+ // while EntryImpl.create retains its own, so this ordering keeps the counts balanced.
+ EntryImpl entry = EntryImpl.create(ledgerEntry, 0);
+ ledgerEntry.close();
+ return entry;
+ }
+
+ private static EntryImpl createFromRetainedBuffer(long ledgerId, long entryId) {
+ ByteBuf buffer = Unpooled.wrappedBuffer(new byte[]{1, 2, 3});
+ EntryImpl entry = EntryImpl.create(ledgerId, entryId, buffer);
+ // create(long, long, ByteBuf, int) retains the buffer; drop the caller-owned reference.
+ buffer.release();
+ return entry;
+ }
- second.getPosition(); // re-poison the recycled object
- EntryImpl third = EntryImpl.create(7L, 30L, Unpooled.wrappedBuffer(new byte[]{7, 8}));
- assertTrue(third.getPosition().compareTo(PositionFactory.create(7L, 30L)) == 0,
- "ByteBuf variant: a recycled entry must not inherit the poisoned (-1, -1) position");
- third.release();
+ private static boolean warmUpRecyclerUntilReused() {
+ // Absorb Netty's io.netty.recycler.ratio warm-up and prove the per-thread pool hands the
+ // same instance back; without it the assertions above could be checking fresh objects.
+ for (int i = 0; i < 64; i++) {
+ EntryImpl warm = EntryImpl.create(0L, 0L, new byte[]{0});
+ warm.release();
+ EntryImpl again = EntryImpl.create(0L, 0L, new byte[]{0});
+ boolean reused = again == warm;
+ again.release();
+ if (reused) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static void runOnFastThreadLocalThread(Runnable scenario) throws Exception {
+ CompletableFuture<Void> completion = new CompletableFuture<>();
+ Thread thread = new FastThreadLocalThread(() -> {
+ try {
+ scenario.run();
+ completion.complete(null);
+ } catch (Throwable error) {
+ completion.completeExceptionally(error);
+ }
+ });
+ thread.start();
+ // Rethrow assertion failures on the test thread.
+ completion.get(30, TimeUnit.SECONDS);
Review Comment:
`completion.get(30, TimeUnit.SECONDS)` returns as soon as the future is resolved, but the thread is technically still alive until its `run()` returns. The gap is tiny, but adding `thread.join()` after `completion.get(...)` makes the lifecycle explicit and prevents any thread-leak warnings from strict test frameworks:
```java
completion.get(30, TimeUnit.SECONDS);
thread.join();
```
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/EntryImplTest.java:
##########
@@ -260,29 +266,100 @@ public void testCreateWithPositionThatIsImmutable() {
}
@Test
- public void testRecycledObjectDoesNotInheritPoisonedPosition() {
+ public void testRecycledObjectDoesNotInheritPoisonedPosition() throws Exception {
+ // Netty's Recycler only pools instances for FastThreadLocalThreads; a plain test worker
+ // thread receives a no-op handle, every create() would return a fresh object, and this
+ // scenario would silently degrade to asserting fresh instances (see the review on #26707).
+ // Run it on a FastThreadLocalThread and prove instance identity with assertSame.
+ runOnFastThreadLocalThread(() -> {
+ assertTrue(warmUpRecyclerUntilReused(),
+ "recycler never handed the same instance back; the scenario cannot exercise recycling");
+
+ // The managed-ledger read path wraps a BookKeeper LedgerEntry.
+ assertRecycledPoisonedPositionIsNotInherited("create(LedgerEntry, int)",
+ () -> createFromLedgerEntry(5L, 10L),
+ () -> createFromLedgerEntry(6L, 20L), 6L, 20L);
+ // byte[] variant.
+ assertRecycledPoisonedPositionIsNotInherited("byte[] variant",
+ () -> EntryImpl.create(5L, 10L, new byte[]{1, 2, 3}),
+ () -> EntryImpl.create(6L, 20L, new byte[]{4, 5, 6}), 6L, 20L);
+ // ByteBuf variant.
+ assertRecycledPoisonedPositionIsNotInherited("ByteBuf variant",
+ () -> createFromRetainedBuffer(5L, 10L),
+ () -> createFromRetainedBuffer(6L, 20L), 6L, 20L);
+ });
+ }
+
+ private static void assertRecycledPoisonedPositionIsNotInherited(String variant,
+ Supplier<EntryImpl> firstEntry,
+ Supplier<EntryImpl> secondEntry,
+ long expectedLedgerId,
+ long expectedEntryId) {
// Given a legitimate entry that is released normally
- EntryImpl first = EntryImpl.create(5L, 10L, new byte[]{1, 2, 3});
+ EntryImpl first = firstEntry.get();
first.release();
// When a getPosition() call slips in AFTER the release: deallocation nulls the lazy
// position field, so this late reader re-materializes it from the reset ids as (-1, -1)
// and leaves the poisoned value cached inside the pooled object.
first.getPosition();
- // Then the next create() (the recycler hands back the most recently released object on
- // the same thread) must not report that stale (-1, -1) position as its own — through
- // both the byte[] and the ByteBuf variants, which own the lazy field.
- EntryImpl second = EntryImpl.create(6L, 20L, new byte[]{4, 5, 6});
- assertTrue(second.getPosition().compareTo(PositionFactory.create(6L, 20L)) == 0,
- "byte[] variant: a recycled entry must not inherit the poisoned (-1, -1) position");
+ // Then the very next create() on this thread reuses that exact instance and must not
+ // report the poisoned (-1, -1) position as its own.
+ EntryImpl second = secondEntry.get();
+ assertSame(second, first, variant + ": expected the recycler to reuse the same instance");
+ assertTrue(second.getPosition().compareTo(PositionFactory.create(expectedLedgerId, expectedEntryId)) == 0,
Review Comment:
`assertTrue(x.compareTo(...) == 0, msg)` produces a failure message like `expected true but got false`. Swapping to `assertEquals` would print the actual and expected positions on failure, which is much more useful when debugging:
```java
assertEquals(second.getPosition(), PositionFactory.create(expectedLedgerId, expectedEntryId),
variant + ": a recycled entry must not inherit the poisoned (-1, -1) position");
```
The `compareTo`-based form is already used elsewhere in this file, so this isn't blocking, but it's worth changing in a test whose whole point is catching bad values.
--
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