anxkhn opened a new pull request, #17150:
URL: https://github.com/apache/iceberg/pull/17150
### Problem
On the split-read path, `AvroIterable.iterator()` opens a `DataFileReader`
via
`newFileReader()` and then wraps it in an `AvroRangeIterator`:
```java
FileReader<D> fileReader = initMetadata(newFileReader()); // opens the
reader + stream
if (start != null) {
...
fileReader = new AvroRangeIterator<>(fileReader, start, end); // can
throw
}
addCloseable(fileReader); // ownership handed to CloseableGroup here
```
The `AvroRangeIterator` constructor calls `reader.sync(start)` and rethrows
any
`IOException` as a `RuntimeIOException`. Because the reader is registered
with the
`CloseableGroup` (`addCloseable`) only *after* the wrapper is built, a
failure in
that `sync(start)` (for example a truncated or corrupt data file, or an I/O
error
seeking to the split offset) propagates out of `iterator()` before
`addCloseable()`
runs. The already-open `DataFileReader` and its underlying stream are never
registered 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 makes `DataFileReader.openReader`
throw and
leak the input stream). That fix did not cover the split path, where the
throw
originates in the range iterator's `sync` instead.
### Solution
Guard the wrapper construction so the already-open reader is closed if
`AvroRangeIterator` throws, then rethrow:
```java
try {
fileReader = new AvroRangeIterator<>(fileReader, start, end);
} catch (RuntimeException e) {
try {
fileReader.close();
} catch (IOException closeException) {
e.addSuppressed(closeException);
}
throw e;
}
```
At the throw point the assignment has not completed, so `fileReader` still
references the base `DataFileReader`; `close()` therefore targets the correct
resource. Any secondary failure while closing is attached with
`addSuppressed` so it
does not mask the original error. On success nothing changes: `addCloseable`
runs as
before and the final `CloseableGroup` state is identical. Only the failure
path now
cleans up. This mirrors the cleanup #14322 added for `newFileReader()`.
### Testing
Added a regression test `readerClosedWhenRangeSyncFails` in
`core/src/test/java/org/apache/iceberg/avro/TestAvroIterable.java`. It opens
an
`AvroIterable` with a split (`start`/`length` set) over a mocked reader whose
`sync(start)` throws `IOException`, asserts `iterator()` fails with
`RuntimeIOException`, and verifies the reader was closed. It follows the
existing
`streamClosedOnIOException` test in the same class (`mockStatic` on `AvroIO`
and
`DataFileReader`).
- `./gradlew :iceberg-core:test --tests
"org.apache.iceberg.avro.TestAvroIterable"`
passes (2 tests, 0 failures).
- Confirmed the test is a genuine regression: reverting only the source
`try/catch`
makes `readerClosedWhenRangeSyncFails` fail with
`Wanted but not invoked: dataFileReader.close()`; restoring it turns green
again.
- `./gradlew :iceberg-core:spotlessCheck` passes (formatting clean).
---
**AI Disclosure**
- Model: Claude Opus 4.8
- Platform/Tool: opencode
- Human Oversight: fully reviewed
- Prompt Summary: Find and fix a resource leak in Iceberg core. Identified
that
`AvroIterable.iterator()` leaks the open `DataFileReader`/stream on the
split-read
path when `AvroRangeIterator`'s `sync(start)` fails before the reader is
registered
with the `CloseableGroup`, extended the cleanup from #14322 to that path,
and added
a 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 the
`AvroRangeIterator` construction in `try/catch (RuntimeException)` that
closes the
already-open `fileReader` (suppressing any close `IOException`) and
rethrows.
- `core/src/test/java/org/apache/iceberg/avro/TestAvroIterable.java` (+30):
new
regression test `readerClosedWhenRangeSyncFails`.
Total: 2 files, +40 / -1.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
