PDGGK opened a new pull request, #9258:
URL: https://github.com/apache/paimon/pull/9258
### Purpose
`RowFormatReaderFactory#createReader` opens the input stream on its first
line and hands ownership to `RowFormatReader` on its last. Everything in
between parses offsets and lengths taken out of the file itself, and any of it
can throw on a truncated or corrupt file:
```java
SeekableInputStream in = fileIO.newInputStream(path);
in.seek(tailOffset);
IOUtils.readFully(in, tailBuf);
RowFileFooter footer = RowFileFooter.readFrom(tailBuf, tailSize -
RowFileFooter.FOOTER_SIZE);
...
byte[] indexData = new byte[footer.indexLength]; //
length from the file
System.arraycopy(tailBuf, indexOffsetInBuf, indexData, 0,
footer.indexLength);
blockIndex = RowBlockIndex.readFrom(indexData);
return new RowFormatReader(in, ...); //
ownership passes here
```
Concretely:
* a corrupt or truncated tail — `RowFileFooter.readFrom` throws
`IOException("Invalid row file magic: …")`;
* a zero-length file, which is what an aborted write leaves behind —
`tailSize` is 0, so the footer is read at offset `-32` and the array access
throws;
* a bad `indexLength` or `indexOffset` — `new byte[…]` or the `arraycopy`
throws.
On every one of those, `in` is a local that was never handed out, so nothing
can close it.
### Why this repeats rather than happening once
`DataFileRecordReader.createReader` wraps this call:
```java
try {
return readerFactory.createReader(context);
} catch (Exception e) {
...
if (ignoreCorruptException(e, ignoreCorruptFiles)) {
LOG.warn("Failed to create FileRecordReader for file: {}, ignore
exception", ...);
return null; // and the scan moves on
}
```
and `ignoreCorruptException` accepts `IOException`, `RuntimeException` and
`InternalError` — which covers all of the failures above. So with
`ignore-corrupt-files` enabled, a scan over a directory containing corrupt
`row` files leaks one descriptor per file and keeps going, which is exactly the
configuration in which nobody notices.
### What changes
The body is wrapped so the stream is closed if ownership never transfers:
```java
try {
...
return new RowFormatReader(in, ...);
} catch (Throwable t) {
IOUtils.closeQuietly(in);
throw t;
}
```
`Throwable` rather than `Exception` because `new byte[footer.indexLength]`
with a corrupt length can raise an `Error`, and the caller's ignorable set
already includes `InternalError`. `IOUtils` was already imported here for
`readFully`.
Not try-with-resources: on the success path the stream must stay open, since
`RowFormatReader` owns it from then on and closes it through `BlockPrefetcher`.
### Blast radius
None on a well-formed file — the only added path is the catch, and the
success path returns exactly as before. `RowFormatReadWriteTest` (27) and
`BlockPrefetcherTest` (14) pass unchanged.
### Tests
`RowFormatReaderFactoryLeakTest`, two cases, over a `LocalFileIO` subclass
that counts input streams opened and not yet closed: a 128-byte file of zeros
(fails the magic check) and a zero-length file (fails in the footer read). Both
assert the count is back to zero after the failure.
Removing the `closeQuietly` fails both:
```
Tests run: 2, Failures: 2, Errors: 0
aCorruptFooterDoesNotLeakTheInputStream
anEmptyFileDoesNotLeakTheInputStream
```
`RowFormatReaderFactoryLeakTest`, `RowFormatReadWriteTest` and
`BlockPrefetcherTest`: 43 tests, 0 failures. `spotless:apply` and
`checkstyle:check` on `paimon-format` are clean.
### API and Format
No change to any public signature, option or on-disk format.
--
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]
