Repository navigation
camel-file: do not assume skip() advanced the whole offset - #29702
Open
Dhairya-007 wants to merge 2 commits into
Open
Dhairya-007 wants to merge 2 commits into
Dhairya-007 wants to merge 2 commits into
Conversation
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
reviewed
Nov 12, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
TL;DR: the skip loop is correct and cannot spin, but the new EOF path leaks the opened file, and the behaviour change needs an upgrade-guide entry.
The skipFully loop treats a 0 return as EOF, which matches the Reader.skip contract (it blocks until characters are available or EOF). skipNBytes probes with read(), and on the no-charset path Files.newInputStream clamps skip at the file size, so the EOF case really is detected now. The FilterReader-based tests show the short-skip case nicely.
-
Close on failure. In
GenericFileConverter.genericFileToInputStream(lines 126-129) the stream is opened and thenskipNBytescan throwEOFException; the stream is never returned and never closed. The same goes for the reader ingenericFileToReader(lines 201-207). InIOHelper.toString(BufferedReader, long),skipFullyruns before thetrywhosefinallycloses the reader. Before this change these paths practically never threw; with a stale offset they now do on every attempt, leaking a file handle each time (and on Windows blocking the move or delete). Something like:InputStream inputStream = IOHelper.toInputStream(f, charset); try { inputStream.skipNBytes(file.getLastOffsetValue()); } catch (IOException e) { IOHelper.close(inputStream); throw e; } return inputStream;
plus the same for the reader, and moving
skipFully(reader, offset)inside the existingtryinIOHelper.toString. -
Upgrade guide: yes, please add the entry you offered in
camel-4x-upgrade-guide-4_24.adoc: a stale offset now fails the exchange instead of giving an empty body. As far as I can tell, a failed exchange leaves the file in place and the next poll reloads the same stale offset from the resume cache, so the failure repeats on every poll; users with truncated or rotated files will want to know that, and how to reset the offset. -
Optional: a camel-file resume test with an offset larger than the file would pin the converter behaviour, and that the file is released afterwards.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 593 of 723 tested, 27 compile-only — current: 593 all testedMaveniverse Scalpel detected 593 affected modules (current approach: 593). Skip-tests mode would test 593 modules (2 direct + 591 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (593)
Modules with tests skipped (27)
Build reactor — dependencies compiled but only changed modules were tested (2 modules, 16.6s total)Total reactor time: 16.6s
Top 20 slowest modules:
|
oscerd
reviewed
Nov 12, 2026
oscerd
left a comment
oscerd
left a comment
Contributor
There was a problem hiding this comment.
Confirmed the bug and that the sweep is complete.
InputStream.skip(long) and Reader.skip(long) both skip up to the requested amount and return how far they actually got, so a single unchecked skip(offset) on the file-resume path can leave the body starting partway into the data the recorded offset was meant to skip over. That is a real correctness issue for resume, and more likely the larger the offset or the more buffered the stream.
The fixes match the API each call has:
inputStream.skip(...)becomesinputStream.skipNBytes(...), which the JDK already defines as skip-exactly-or-throw, and which falls back toread()internally. That fallback is exactly why a plainskipwas not enough for anInputStream.- The two reader calls go through the new
IOHelper.skipFully, sinceReaderhas noskipNBytes. The loop is right:Reader.skipblocks until a character is available or the stream ends, so reading a<= 0return as end-of-stream and throwingEOFExceptionmatches the contract, and the runningremaininglets the message report how far it actually got.
I grepped camel-file and IOHelper for other unchecked skip( on the offset path, and these three (GenericFileConverter twice, IOHelper.toString) are all of them, so nothing in this family is left on the old pattern.
The tests cover the part that matters: a reader that only skips one character per call still lands exactly, and skipping past the end throws. A FilterReader that returns super.skip(1) is a faithful stand-in for a short-skipping reader.
Claude Code on behalf of oscerd
The file resume support positions the body at the recorded offset with a
single unchecked call:
inputStream.skip(file.getLastOffsetValue()); // GenericFileConverter
reader.skip(file.getLastOffsetValue());
reader.skip(offset); // IOHelper.toString
InputStream.skip and Reader.skip are both specified to skip up to the
requested amount and to return how far they actually got, so the return
value is the only thing that says where the stream now is. Discarding it
means the body silently starts somewhere other than the offset asked for.
The case reachable today is an offset past the end of the file, which
happens when the recorded offset is stale because the file was truncated or
replaced: the stream ran to EOF and the exchange carried an empty or short
body as if that were the file's content, instead of reporting that the
offset could not be honoured.
The stream now uses InputStream.skipNBytes, which loops and throws
EOFException if the stream ends first, and readers go through the new
IOHelper.skipFully, which does the same for Reader.skip.
Review follow-up on apache#29702. The new EOFException path leaked the open file: in GenericFileConverter.genericFileToInputStream and genericFileToReader the stream is opened and then skipped, and when the skip throws, the stream is neither returned nor closed - so a stale offset leaked a handle on every poll, which on Windows also blocks the move or delete that follows. Both now close it and rethrow. In IOHelper.toString the skip moves inside the try whose finally already closes the reader. Adds the upgrade-guide entry: a stale offset fails the exchange instead of giving an empty body, and the failure repeats until the recorded offset is reset. GenericFileConverterResumeOffsetTest covers both converters skipping to the offset, and an offset past the end of the file failing and releasing the file.
Dhairya-007
force-pushed
the
fix/file-resume-partial-skip
branch
from
November 12, 2026 13:43
b61fef8 to
9fdf9ae
Compare
Contributor
Author
|
All three points are in, thanks — the leak is the important one and I had not thought it through: before this change those paths practically never threw, so opening-then-skipping was harmless; with a stale offset it throws on every poll. 1. Close on failure. All three places: InputStream inputStream = IOHelper.toInputStream(f, charset);
try {
inputStream.skipNBytes(file.getLastOffsetValue());
} catch (IOException e) {
// the stream is not returned, so nothing else can close it
IOHelper.close(inputStream);
throw e;
}
return inputStream;the same for the reader in 2. Upgrade guide — added, and it says what you pointed out about the repeat: the failing exchange leaves the file in place, so the next poll reads the same offset from the resume store and fails again, and the recorded offset has to be reset for the file to be consumed from the start. 3. The test you suggested — added as Also rebased on main, since the branch was 58 commits behind and the upgrade-guide entry would otherwise have landed in a conflicting spot (same situation as #29709). |
gnodet-bot
reviewed
Nov 12, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
All three issues flagged by @davsclaus are now addressed: skipNBytes / skipFully with proper close-on-failure in both converter paths, an upgrade-guide entry, and unit tests covering the stale-offset failure and file release. LGTM.
A few notes on the implementation:
IOHelper.skipFully — the loop is correct. Reader.skip blocks until characters are available or EOF, so a return value of ≤ 0 means genuine EOF, not a transient empty read. The EOFException message reports how far the reader got (offset - remaining), which aids debugging.
GenericFileConverter — both genericFileToInputStream and genericFileToReader now close and rethrow on a stale offset. The InputStream path correctly uses skipNBytes (JDK built-in, loops and throws EOFException). The Reader path correctly delegates to skipFully. Both methods close on IOException and rethrow — no handle leak on Windows on the failing-poll cycle.
IOHelper.toString(BufferedReader, long) — skipFully is now inside the existing try/finally, so the reader is closed on a stale offset here too.
Tests — GenericFileConverterResumeOffsetTest covers both converters for the happy path and the stale-offset failure path, and asserts the file can be deleted after the failure (the Windows-critical check). IOHelperTest adds a OneAtATimeSkipReader fixture that proves the loop handles short skips.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
reviewed
Nov 12, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks, this addresses everything from my last review: genericFileToInputStream and genericFileToReader close the stream/reader when the skip fails (GenericFileConverter.java:126-135, 213-219), IOHelper.toString(BufferedReader, long) skips inside the try that closes the reader (IOHelper.java:169-182), and there is a converter-level test plus an upgrade-guide entry. I checked the blast radius of the IOHelper change: every caller of toString(Reader|BufferedReader, ...) in the repo goes through the one-argument form with offset 0, where skipFully is a no-op, so only the camel-file resume path changes behaviour.
A few small wording points in the upgrade-guide entry:
- "the exchange carried an empty or short body": for an offset past the end the old code produced an empty body; a short
skip()would leave the body starting earlier (longer, not shorter). Maybe "an empty body as if that were the file's content". - "the failing exchange leaves the file in place": true by default, but with
moveFailedthe file is moved. "By default (withoutmoveFailed)" avoids surprising those users. - Optional: the offset counts characters when the body is read as
String/Readerand bytes when read asInputStream/byte[]. Not new, but a byte offset over multi-byte content now fails instead of silently giving an empty body, so one sentence on the unit could save someone a debugging session.
Optional in the test: the Files.delete(...) check after the failure only catches a leaked handle on Windows (Linux and macOS allow deleting an open file). A short comment saying so keeps the next reader from relying on it.
Heads-up: #29709 also appends to the end of the 4.24 upgrade guide, so whichever merges second needs a quick rebase.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The file resume support positions the body at a recorded offset with a single unchecked call, in three places:
InputStream.skip(long)andReader.skip(long)are both specified to skip up to the requested amount and to return how far they actually got — the javadoc of the former says it "may, for a variety of reasons, end up skipping over some smaller number of bytes, possibly 0". The returned count is the only thing that says where the stream now is, and all three calls discard it, so the body can silently start somewhere other than the offset that was asked for.How reachable is it?
Honestly: for a fresh local file the skip normally completes, because
BufferedInputStream.skipdelegates to the position-based skip of the channel stream, andReader.skiploops internally. I could not make a plain local file skip short mid-stream.The case that is reachable is an offset past the end of the file, which is exactly what a stale recorded offset looks like after the file was truncated or replaced. Today the stream runs to EOF and the exchange carries an empty or short body as if that were the file's content; nothing says the offset could not be honoured. For a resume feature, silently delivering the wrong bytes is worse than failing.
Beyond that, the code is relying on an implementation detail of the stream it happens to get.
IOHelper.toInputStreamreturns aBufferedInputStreamfor the no-charset path and aReaderInputStreamfor the charset path, andGenericFileConverterdoes not control which stream a caller ends up with.Fix
GenericFileConverter.genericFileToInputStreamusesInputStream.skipNBytes(long)(JDK 12+), which loops and throwsEOFExceptionif the stream ends first.IOHelpergainsskipFully(Reader, long), the same loop for readers, sinceReaderhas noskipNBytes. It is used byGenericFileConverter.genericFileToReaderand byIOHelper.toString(BufferedReader, long).Behaviour change
An offset past the end of the file now fails with
EOFExceptioninstead of yielding an empty body. That is the point of the change, but it is a visible difference for anyone whose resume offsets can go stale. If you would rather have an upgrade-guide entry for it, say so and I will add one.Tests
IOHelperTestgets two cases driving aFilterReaderthat skips one character per call — whichReader.skipis free to do, and which a single unchecked call has to cope with:testSkipFullyWithAReaderThatSkipsInSmallSteps— four one-character skips add up to the requested four, and the rest of the content is intact. A barereader.skip(4)on that reader advances one character, so the old code would return123456789.testSkipFullyPastTheEnd—EOFExceptionrather than a silent short read.