nhooey opened a new pull request, #1362: URL: https://github.com/apache/arrow-java/pull/1362
## What's Changed `BufferLedger.retain` now uses a `compareAndSet` loop that never moves the count off 0, so a `retain` on a released buffer keeps throwing instead of reviving freed memory. The second commit closes the same hole in `retain(ArrowBuf, BufferAllocator)` and `transferOwnership`, which returned a usable buffer over freed memory. `release` is 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 `retain` can see the positive count before the reset (the Tests table). The Benchmark section measures the loop's cost. ### Fix from XTDB The `retain` fix is @jarohen's: XTDB has shipped it in its patched `BufferLedger` since xtdb/xtdb@5576310378e4a984c3161c06cb4f1979bf4ac0b9, after the bug crashed its JVM with a `SIGSEGV` (xtdb/xtdb#5013). This PR brings it upstream with tests and a benchmark, and differs from [XTDB's `retain`](https://github.com/xtdb/xtdb/blob/5576310378e4a984c3161c06cb4f1979bf4ac0b9/core/src/main/java/org/apache/arrow/memory/BufferLedger.java#L172-L187) only where `main` has moved on since XTDB's copy: it uses the `AtomicIntegerFieldUpdater` from GH-1038, and keeps the historical-log call before the loop, so a failed `retain` still shows when `BaseAllocator.DEBUG` is on. The cross-allocator fix is new. ### Tests The first commit's tests, repeated locally with `@RepeatedTest`: | Test | `main` | reset before throw | this PR | |---|---|---|---| | `testRetainAfterCloseThrowsEveryTime` | 50/50 fail | pass | 500/500 pass | | `testFailedRetainDoesNotReviveReleasedMemory` | 50/50 fail | pass | 500/500 pass | | `testRetainRacingFinalReleaseNeverRevivesBuffer` | 50/50 fail | 48/500 fail | 500/500 pass | Reset-before-throw's 48 failures were 26 `retain`s succeeding after the buffer was freed, and 22 second `release`s to 0 failing in `AllocationManager.release` with `Expecting 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. The full default build passes (`mvn verify`: 22 modules, 5,259 tests, among them `vector`'s and Flight's, which transfer buffers between allocators), as do `spotless` and `checkstyle`. ### Benchmark `BufferLedgerBenchmarks`, average ns per operation: JMH 1.37 with `-wi 5 -w 1s -i 5 -r 1s` and 10 forks (5 for 4 and 8 threads), on an Apple M2 Pro (AArch64; x86 wasn't measured) with Zulu JDK 21, both jars built from the same tree and run back to back; ± is JMH's 99.9% confidence interval. | Benchmark | `main` | this PR | |---|---|---| | `retainReleaseUncontended` | 16.36 ± 0.13 | 18.00 ± 0.19 | | `retainReleaseContended2` | 82.25 ± 4.92 | 110.01 ± 14.81 | | `retainReleaseContended4` | 243.54 ± 19.57 | 247.06 ± 16.14 | | `retainReleaseContended8` | 470.41 ± 38.87 | 497.64 ± 32.62 | | `retainToAllocatorClose` | 63.25 ± 3.28 | 62.51 ± 1.74 | Uncontended, the loop adds about 1.6 ns per `retain` + `release`, against about 115 ns for `allocateClose`. At 2 threads a `retain` that 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](https://claude.com/claude-code) -- 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]
