Skip to content

[fs] Delegate skipFully to hadoop's IOUtils - #9779

Open
LuciferYang wants to merge 7 commits into
apache:masterfrom
LuciferYang:fix/skipfully-zero-skip-hang
Open

[fs] Delegate skipFully to hadoop's IOUtils#9779
LuciferYang wants to merge 7 commits into
apache:masterfrom
LuciferYang:fix/skipfully-zero-skip-hang

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Purpose

close #9778

HadoopSeekableInputStream.seek turns a small forward gap (up to MIN_SKIP_BYTES, 1 MiB) into skipFully rather than a real seek, which for an object store avoids tearing down and reopening the connection. The loop it used was

while (bytes > 0) {
    bytes -= in.skip(bytes);
}

and skip returning 0 makes no progress, so a seek past the end of a truncated or shorter-than-expected file spun forever. A hang is the worst way for this to fail: nothing to catch, nothing in the logs.

The first version of this fix threw EOFException on the first zero-byte skip. That is not quite right, and it is also work already done upstream. InputStream.skip is documented to return 0 for reasons other than EOF, so a zero has to be resolved by reading, which is exactly what org.apache.hadoop.io.IOUtils.skipFully does:

long ret = in.skip(amt);
if (ret == 0) {
  int b = in.read();
  if (b == -1) { throw new EOFException(...); }
  ret = 1;
}

It ships in hadoop-common, which every module here already depends on for FSDataInputStream, so this delegates to it. That fixes the detection and removes eight byte-identical copies of the loop at the same time: the net change is 96 lines deleted against 40 added, where the first version added 144.

For the streams actually in play a zero does mean EOF (FSInputStream does not override skip, so it inherits the read-based InputStream.skip, and BufferedFSInputStream.skip returns n unconditionally for n > 0), so this is not a live data bug on any filesystem in the tree. It matters for the wrappers, which adapt arbitrary Hadoop filesystems including ones outside this repo.

Tests

HadoopFileIOSkipFullyTest covers the three cases the loop got wrong or right by accident:

  • a stream at the end: skip returns 0 and the probe reads -1, so it throws EOFException.
  • a transient zero: skip returns 0 once and then makes progress. This is the case the first version of this fix broke.
  • nothing to skip: no call to skip at all.

Two details of how they assert, both of which took a mutant to get right. The first stub returns 0 once and then throws, rather than returning 0 forever: against the loop this replaces that turns an unbounded spin into a failed assertion naming it, so the test reports the bug instead of hanging the fork. A @Timeout cannot do that job here, since Jupiter's default thread mode is SAME_THREAD and threadMode arrived in 5.9 while this build is on 5.8.1. And the transient-zero case pins the whole conversation in order — skip(4096), read(), skip(4095) — with verifyNoMoreInteractions, because each weaker form let a real accounting bug through: verify(read()) alone passes a delegate that probes without crediting the probed byte, adding verify(skip(4095)) still passes one that credits the probe but under-asks by one up front, and the in-order chain alone does not constrain totals, so a delegate probing twice passes it.

Verified on JDK 11. Three tests pass; each fails for a reason the other two do not. Reverting skipFully to the loop on master fails two of them with skip was called again after returning 0; restoring the fail-fast version fails two; and the three accounting mutants above each fail exactly one assertion. skipFully is byte-identical in all 17 hadoop-common jars from 2.2.0 to 3.4.3, so 4095 is the right number for every version this can run against, and the interaction set cannot grow without a change to that method. All eight touched modules (paimon-common plus the seven filesystem impls) build with checkstyle and spotless enabled.

skipFully looped while (bytes > 0) subtracting whatever in.skip
returned. A blocking stream at end of file returns 0 from skip, so
a small forward seek (up to 1 MiB, used by the vectored-read
helpers) past a truncated or shorter-than-expected file spun the
loop forever instead of failing the read with an end-of-file error.

Throw EOFException when a skip call makes no progress. The same
loop was replicated in HadoopFileIO and in every
HadoopCompliantFileIO wrapper (azure, cosn, gs, jindo, obs, oss,
s3); all copies are updated.

Assisted-by: GLM-5.3
@LuciferYang
LuciferYang marked this pull request as draft September 13, 2026 03:07
LuciferYang and others added 2 commits September 13, 2026 15:33
The hand-rolled loop declared EOF on the first zero-byte skip, but
InputStream.skip may return 0 without being at the end, and the issue's own
reference, org.apache.hadoop.io.IOUtils.skipFully, resolves that by probing
with read(). It ships in hadoop-common, which every one of these modules
already depends on for FSDataInputStream, so delegating both fixes the
detection and removes eight byte-identical copies of the loop.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
@LuciferYang LuciferYang changed the title [fs] Fail fast on zero-byte skip in HadoopCompliantFileIO wrappers [fs] Delegate skipFully to hadoop's IOUtils Sep 13, 2026
…loop did

Bounds the stub so a looping caller fails instead of hanging the fork,
pins skip-probe-skip in order with no other interaction, and fixes two
comments that claimed more than any assertion checked.
@LuciferYang
LuciferYang marked this pull request as ready for review September 13, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] skipFully spins forever on a zero-byte skip at end of stream

1 participant