createcentury commented on PR #51603: URL: https://github.com/apache/arrow/pull/51603#issuecomment-6128590526
This looks correct to me, and the regression test is the part I'd highlight. **The fix.** The deadlock in #51547 comes from `reopen_reader` calling the synchronous `OpenReader`, which reaches `RecordBatchFileReaderImpl::ReadFooter()` — a blocking `ReadFooterAsync(nullptr).status()`. Because `MergedGenerator::State::PullSource()` holds the state mutex across the call into the source generator, that blocking wait happens while the lock is held, and every completion callback that needs the same mutex is stuck behind an IO read that can no longer be serviced. Switching to `OpenReaderAsync` removes the wait entirely rather than making it less likely, so the lock is never held across an IO round trip. The two overloads take the same arguments and `reopen_reader` already returns `Future<...>`, so nothing else has to change — and the initial open a few lines above already goes through `OpenReaderAsync`, so this just makes the re-open consistent with it. **The test.** `GatedIpcBufferReader` is a nice way to handle this. Rather than trying to race the original deadlock, which would be inherently flaky, it holds the second open's first `ReadAsync` as an unfinished `Future` and asserts that `ScanBatchesAsync` returns while that IO is still outstanding. That tests the actual invariant the fix establishes — no synchronous wait on the reopen path — and it fails deterministically if that invariant is ever lost. Returning a different reader on the second open through the `FileSource` factory lambda targets exactly the path in question, which is a detail that's easy to get wrong. CI is green across the board here — 43 passing, 16 skipped, nothing failing, including `ARM64 macOS 14 C++` and `AMD64 Ubuntu 24.04 C++ ASAN UBSAN`. This has been sitting since September 28th without a review. Could a committer take a look? The analysis in #51547 is unusually thorough and the fix that came out of it is a one-line change. -- 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]
