DanielLeens commented on PR #11593:
URL: https://github.com/apache/seatunnel/pull/11593#issuecomment-5178093128
Following up after @SEZ9's review — and I need to correct my own previous
comment.
In my last note I wrote that I did not see a code-side blocker on this head
and that the remaining gate was CI. **That was wrong, and @SEZ9 is right.** I
re-read `close()` on the current head and their Issue 1 is real:
```java
public void close() {
try {
if (arrowStreamReader != null) { arrowStreamReader.close(); root =
null; }
if (byteArrayInputStream != null) { byteArrayInputStream.close(); }
if (rootAllocator != null) { rootAllocator.close(); }
} catch (IOException e) {
throw new RuntimeException("failed to close arrow stream reader.",
e);
}
}
```
All three closes share one `try`. If `arrowStreamReader.close()` throws
`IOException`, control leaves for the `catch` and **`rootAllocator.close()`
never runs** — so the Arrow root allocator, which owns off-heap native memory,
is leaked for the lifetime of the JVM. That is the exact failure mode this PR
set out to fix; the ordering is now correct, but the failure path is not. +1 to
@SEZ9 on treating it as blocking.
The evidence I want to add on top of their finding is *why this matters more
than it looks*: this is a source-side reader on a long-lived streaming path, so
the leak is per-reader-instance and cumulative, and native allocator memory
does not show up in heap dumps — the symptom is a container OOM-kill with a
healthy-looking JVM heap. That is a genuinely hard bug to trace back here,
which is why I would rather fix the failure path now than merge the ordering
fix alone.
Suggested shape — each close independent, first failure preserved, later
failures attached rather than lost:
```java
public void close() {
Throwable primary = null;
try {
if (arrowStreamReader != null) { arrowStreamReader.close(); root =
null; }
} catch (Throwable t) { primary = t; }
try {
if (rootAllocator != null) { rootAllocator.close(); }
} catch (Throwable t) {
if (primary == null) { primary = t; } else {
primary.addSuppressed(t); }
}
if (primary != null) {
throw new RuntimeException("failed to close arrow reader
resources.", primary);
}
}
```
Note this also covers @SEZ9's Issue 3 — `rootAllocator.close()` throws
`IllegalStateException` (unchecked) when Arrow's leak detector finds
outstanding buffers, and that currently bypasses the `catch (IOException)`
entirely and propagates with no context about which reader leaked. Catching
`Throwable` rather than `IOException` in the allocator branch fixes both at
once. Their Issue 6 is also right that the explicit `byteArrayInputStream`
close is redundant, since `ArrowStreamReader.close()` already closes the
underlying stream and `ByteArrayInputStream.close()` is a no-op — dropping it
makes the method shorter, though that one is cosmetic.
I am also +1 on their **Issue 2**: a regression test for a resource-cleanup
fix that never calls `reader.close()` and asserts nothing about the allocator
does not actually guard the behavior. The assertion that would have caught this
bug is `rootAllocator.getAllocatedMemory() == 0` after `close()`, ideally with
a variant where the underlying reader is made to throw so the failure path is
exercised too. Without that, the same regression can return silently.
On the earlier reviewers' points I have nothing to contradict — I agree with
their severity ordering, and I would treat Issues 4, 5, and 7 as genuinely
non-blocking.
So my technical position on this head has changed from "waiting on CI" to
**blocked on Issues 1 and 2**. Please disregard my previous "the remaining gate
is CI" framing; that was my error, not a change in the code. Happy to re-review
the full head as soon as you push an update.
One correction for the record: I also said previously that I only had
read-only review controls here. That was not accurate — I can submit reviews,
and will do so on the next head.
--
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]
