Repository navigation
Conversation
BufferLedger.retain added to the reference count with getAndAdd and only then checked that the old count was positive. A retain on a released buffer (count 0) threw but left the count at the increment, so the next retain succeeded on memory that had already been freed and possibly reused. Increment with a compareAndSet loop that refuses to move the count off 0, keeping the existing IllegalArgumentException. Resetting the count before throwing would only narrow the window: between the failed add and the reset another thread's retain sees a positive count and succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BufferLedger.retain(ArrowBuf, BufferAllocator) and transferOwnership associated the memory with the target allocator without checking that it was still allocated. On a closed buffer whose memory had been freed, they created a ledger with a count of 1 over the freed memory and returned a usable buffer; closing it then failed in AllocationManager.release. AllocationManager.associate now refuses to add a reference once the memory has been freed (no owning ledger), checked under the lock the final release holds while freeing it. A buffer whose own ledger is released can still be retained while another allocator's ledger keeps the memory alive, as TestBaseAllocator.testAllocator_shareOwnership does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Measure what the fix costs: the compareAndSet loop in retain, which retries when threads contend on one buffer, and the freed-memory check in AllocationManager.associate, against buffer allocation and close for scale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
nhooey
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
October 15, 2026 13:19
|
Thank you for opening a pull request! Please label the PR with one or more of:
Also, add the 'breaking-change' label if appropriate. See CONTRIBUTING.md for details. |
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
1 participant
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.
What's Changed
BufferLedger.retainnow uses acompareAndSetloop that never moves the count off 0, so aretainon a released buffer keeps throwing instead of reviving freed memory. The second commit closes the same hole inretain(ArrowBuf, BufferAllocator)andtransferOwnership, which returned a usable buffer over freed memory.releaseis unchanged: once nothing can lift a count off 0, it can't free the memory twice.lidavidm suggested on #906 resetting the count before throwing. That only narrows the window, because another thread's
retaincan see the positive count before the reset (the Tests table). The Benchmark section measures the loop's cost.Fix from XTDB
The
retainfix is @jarohen's: XTDB has shipped it in its patchedBufferLedgersince xtdb/xtdb@5576310, after the bug crashed its JVM with aSIGSEGV(xtdb/xtdb#5013). This PR brings it upstream with tests and a benchmark, and differs from XTDB'sretainonly wheremainhas moved on since XTDB's copy: it uses theAtomicIntegerFieldUpdaterfrom GH-1038, and keeps the historical-log call before the loop, so a failedretainstill shows whenBaseAllocator.DEBUGis on. The cross-allocator fix is new.Tests
The first commit's tests, repeated locally with
@RepeatedTest:maintestRetainAfterCloseThrowsEveryTimetestFailedRetainDoesNotReviveReleasedMemorytestRetainRacingFinalReleaseNeverRevivesBufferReset-before-throw's 48 failures were 26
retains succeeding after the buffer was freed, and 22 secondreleases to 0 failing inAllocationManager.releasewithExpecting a mapping for allocator and reference manager. The race test takes about 0.1 to 0.3 s. The second commit's two closed-buffer tests fail without its check.Benchmark
BufferLedgerBenchmarks, average ns per operation: JMH 1.37 with-wi 5 -w 1s -i 5 -r 1sand 10 forks (5 for 4 and 8 threads), on an Apple M2 Pro with Zulu JDK 21; ± is JMH's 99.9% confidence interval.mainretainReleaseUncontendedretainReleaseContended2retainReleaseContended4retainReleaseContended8retainToAllocatorCloseUncontended, the loop adds about 1.6 ns per
retain+release, against about 115 ns forallocateClose. At 2 threads aretainthat loses the race retries, so the pair is slower and noisier; many threads updating one buffer's count at once isn't a pattern Arrow code is expected to have. From 4 threads up,release's lock dominates. The cross-allocator check costs nothing measurable.AI assistance (Claude Code) was used during development; the complete diff and test results were reviewed by the author.
Closes #906.
🤖 Generated with Claude Code