Repository navigation
[fix][ml] Reset the lazily-cached position when reusing a recycled EntryImpl - #26707
Conversation
dao-jun
added
type/bug
dao-jun
changed the title
… recycled EntryImpl EntryImpl caches its Position lazily: getPosition() materializes it from the ledger/entry ids on first call and deallocate() nulls it before recycling. A getPosition() call that slips in after the refcount has hit zero re-materializes the field from the already-reset ids as (-1,-1) and leaves the poisoned value cached inside the pooled object. Three create() variants relied on lazy materialization instead of assigning the field explicitly, so a recycled object could be born with a stale (-1,-1) position while its getLedgerId()/getEntryId() report fresh legitimate ids — and since the recycler hands back the most recently released object on the releasing thread, one late getPosition() after release poisons the very next create() on that thread. Reset the lazy field in all three variants, placed after the id assignments so any racy lazy rebuild observes the fresh ids, and pin the behavior with a regression test (create -> release -> late getPosition() -> create again) that fails if either reset line is removed.
847c39b to
89af538
Compare
|
I took a closer look at this after the merge. I think the reset is useful as best-effort hardening for the sequential poison-and-reuse case, but it does not fully address the underlying use-after-release/ownership issue. The reported reproduction requires calling In particular, the reset does not close the concurrent race. The following interleaving is still possible:
Since Fixing the use-after-release/ownership handoff therefore seems like the more appropriate root fix—for example, capture required values before releasing, or retain the entry across an asynchronous handoff and release it when that work completes. The resets can remain as defensive hardening, but should not be considered a complete fix for concurrent post-release access. There is also a regression-test issue: the test does not prove that the same object was recycled. With the current Netty recycler defaults, an ordinary test worker can receive a no-op recycler handle. I verified that this test still passes against the base implementation with the production reset lines absent. The test should run the scenario on a |
… recycle Follow-up to apache#26707 addressing the post-merge review: the merged test ran on a plain test worker thread, which receives a no-op recycler handle on Netty 4.2 (pooling is FastThreadLocalThread-only), so every create() returned a fresh instance and the assertions held with or without the fix. - Run the scenario on a FastThreadLocalThread and prove instance identity with assertSame, guarded by a bounded warm-up that fails the test if the pool never hands the same instance back. - Cover create(LedgerEntry, int), the variant the managed-ledger read path actually uses, in addition to the byte[] and ByteBuf variants. - Reword the reset comment in EntryImpl.create(LedgerEntry, int): the reset closes the sequential poison-and-reuse case; it cannot protect against a getPosition() racing concurrently with create() (unsynchronized fields, no generation check) - post-release access remains a caller bug to fix at the call site. Red/green verified: removing the three position resets turns the test red (first failure on the create(LedgerEntry, int) variant); restoring them is green.
…tryImpl (apache#26707) (cherry picked from commit b27d3ad)
…tryImpl (apache#26707) (cherry picked from commit b27d3ad)
…tryImpl (apache#26707) (cherry picked from commit b27d3ad)
3 participants
Motivation
EntryImplcaches itsPositionlazily:getPosition()materializesPositionFactory.create(ledgerId, entryId)into thepositionfield on first call, anddeallocate()nulls the field before handing the object back to the recycler.However, a
getPosition()call that slips in after the refcount has hit zero (a use-after-release on any consumer side) re-materializes the field from the already-reset ids as(-1, -1)and leaves that poisoned value cached inside the pooled object. When the recycler later hands that object to one of thecreate(...)variants that rely on lazy materialization instead of assigning the field explicitly —create(LedgerEntry, int),create(long, long, byte[], int),create(long, long, ByteBuf, int); all otherRECYCLER.get()sites do set it — the new entry is born with a stale(-1, -1)(or previously-used) position whilegetLedgerId()/getEntryId()report fresh legitimate ids.This is not theoretical: Netty's recycler is LIFO on the releasing thread (
claim()→pollLast()), so a single lategetPosition()after release poisons the object that the very nextcreate()on that thread reuses. On a BookKeeper-backed downstream stack we observed exactly this poison-and-reuse cycle under read pressure: every successfully consumed batch immediately poisoned the object reused by the next batch, delivering entries whosegetPosition()was(-1, -1)while their ids were perfectly valid — a nasty corruption family (ghost positions, wrong cursor advancement, silently skipped data) because every other field of the recycled object is legitimately initialized.A library recycler should not amplify a consumer-side use-after-release into persistent delivery corruption of subsequent, unrelated entries.
Modifications
entry.position = nullin the threecreate(...)variants that relied on lazy materialization. The reset is placed after the id assignments so any racy lazy rebuild always observes the fresh ids.create→release→ lategetPosition()→createagain) through both thebyte[]andByteBufvariants and asserts the recycled entry reports its own position; removing either reset line turns the test red.Verifying this change
./gradlew :managed-ledger:test --tests "org.apache.bookkeeper.mledger.impl.EntryImplTest"— passes (12 tests, including the new regression pin).This change is a trivial rework: no code that was covered by tests has been modified beyond the three reset lines.