Core: Close Avro reader when range sync fails in AvroIterable#17150
Core: Close Avro reader when range sync fails in AvroIterable#17150anxkhn wants to merge 1 commit into
Conversation
On the split-read path, AvroIterable.iterator() opens a DataFileReader via newFileReader() and then wraps it in an AvroRangeIterator. That constructor calls reader.sync(start) and rethrows any IOException as a RuntimeIOException. Because the reader is registered with the CloseableGroup only after the wrapper is built, a sync failure (for example a truncated or corrupt data file, or an I/O error seeking to the split offset) propagates before addCloseable() runs, leaking the open reader and its underlying stream. Guard the wrapper construction so the already-open reader is closed if AvroRangeIterator throws, mirroring the cleanup added for newFileReader() in the non-split path. Add a regression test that fails a range sync and asserts the reader is closed. Generated-by: opencode
| Suppliers.memoize(() -> AvroIO.findStartingRowPos(file::newStream, start))); | ||
| } | ||
| fileReader = new AvroRangeIterator<>(fileReader, start, end); | ||
| try { |
There was a problem hiding this comment.
The guard starts one statement too late: setRowPositionSupplier on line 82 can throw before this try and leak the same reader. ValueReaders.PositionReader.setRowPositionSupplier calls posSupplier.get() eagerly (core/src/main/java/org/apache/iceberg/avro/ValueReaders.java:1303), so the memoized supplier runs AvroIO.findStartingRowPos right there, and that throws InvalidAvroMagicException (core/src/main/java/org/apache/iceberg/avro/AvroIO.java:163) or RuntimeIOException (AvroIO.java:181, AvroIO.java:198) on the same truncated/corrupt file and I/O-error conditions this PR targets.
It is reachable on the same split path: StructReader.setRowPositionSupplier installs a PositionReader whenever _pos is projected (ValueReaders.java:1178-1179), planned readers map _pos to ValueReaders.positions() (ValueReaders.java:320), and DeleteFilter projects MetadataColumns.ROW_POSITION whenever position deletes apply (data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:325).
Move try { above the if (reader instanceof SupportsRowPosition) on line 81 so it covers both throw sites. The catch stays correct either way: fileReader is still the base reader at the first throw site, and after the assignment AvroRangeIterator.close() delegates to the same reader.
| try { | ||
| fileReader.close(); | ||
| } catch (IOException closeException) { | ||
| e.addSuppressed(closeException); |
There was a problem hiding this comment.
No test exercises this line: readerClosedWhenRangeSyncFails leaves close() unstubbed on the DataFileReader mock (core/src/test/java/org/apache/iceberg/avro/TestAvroIterable.java:76), so it returns normally and the inner catch (IOException closeException) is never entered.
Add a case that stubs doThrow(new IOException()).when(fileReader).close(); alongside the failing sync, and assert the suppressed exception on the rethrown RuntimeIOException. api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java:57-60 has the idiom: .extracting(e -> Arrays.asList(e.getSuppressed())).asInstanceOf(InstanceOfAssertFactories.LIST).hasSize(1).
Problem
On the split-read path,
AvroIterable.iterator()opens aDataFileReadervianewFileReader()and then wraps it in anAvroRangeIterator:The
AvroRangeIteratorconstructor callsreader.sync(start)and rethrows anyIOExceptionas aRuntimeIOException. Because the reader is registered with theCloseableGroup(addCloseable) only after the wrapper is built, a failure inthat
sync(start)(for example a truncated or corrupt data file, or an I/O errorseeking to the split offset) propagates out of
iterator()beforeaddCloseable()runs. The already-open
DataFileReaderand its underlying stream are neverregistered and never closed, leaking a file handle / stream.
This is the same open-then-throw leak class that #14322 fixed for the non-split
newFileReader()path (a malformed file makesDataFileReader.openReaderthrow andleak the input stream). That fix did not cover the split path, where the throw
originates in the range iterator's
syncinstead.Solution
Guard the wrapper construction so the already-open reader is closed if
AvroRangeIteratorthrows, then rethrow:At the throw point the assignment has not completed, so
fileReaderstillreferences the base
DataFileReader;close()therefore targets the correctresource. Any secondary failure while closing is attached with
addSuppressedso itdoes not mask the original error. On success nothing changes:
addCloseableruns asbefore and the final
CloseableGroupstate is identical. Only the failure path nowcleans up. This mirrors the cleanup #14322 added for
newFileReader().Testing
Added a regression test
readerClosedWhenRangeSyncFailsincore/src/test/java/org/apache/iceberg/avro/TestAvroIterable.java. It opens anAvroIterablewith a split (start/lengthset) over a mocked reader whosesync(start)throwsIOException, assertsiterator()fails withRuntimeIOException, and verifies the reader was closed. It follows the existingstreamClosedOnIOExceptiontest in the same class (mockStaticonAvroIOandDataFileReader)../gradlew :iceberg-core:test --tests "org.apache.iceberg.avro.TestAvroIterable"passes (2 tests, 0 failures).
try/catchmakes
readerClosedWhenRangeSyncFailsfail withWanted but not invoked: dataFileReader.close(); restoring it turns green again../gradlew :iceberg-core:spotlessCheckpasses (formatting clean).AI Disclosure
AvroIterable.iterator()leaks the openDataFileReader/stream on the split-readpath when
AvroRangeIterator'ssync(start)fails before the reader is registeredwith the
CloseableGroup, extended the cleanup from Core: Explicitly close SeekableInput in the AvroIterable #14322 to that path, and addeda regression test.
Diff summary (for reviewer reference; not part of the PR body)
core/src/main/java/org/apache/iceberg/avro/AvroIterable.java(+10 / -1): wrap theAvroRangeIteratorconstruction intry/catch (RuntimeException)that closes thealready-open
fileReader(suppressing any closeIOException) and rethrows.core/src/test/java/org/apache/iceberg/avro/TestAvroIterable.java(+30): newregression test
readerClosedWhenRangeSyncFails.Total: 2 files, +40 / -1.