Skip to content

[IO-891] Fix BoundedReader.skip(long) accounting and bounds handling #860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
garydgregory merged 3 commits into from
Jun 19, 2026
+40 −2
Merged

[IO-891] Fix BoundedReader.skip(long) accounting and bounds handling #860

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86cd859
[IO-891] Fix BoundedReader skip accounting
Jun 17, 2026
d1e8603
[IO-891] Refine BoundedReader skip bounds fix
Jun 18, 2026
9b8dae3
Fix Checkstyle build failure
garydgregory Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/main/java/org/apache/commons/io/input/BoundedReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,18 @@ public void reset() throws IOException {

@Override
public long skip(final long n) throws IOException {
charsRead += n;
return super.skip(n);
if (n <= 0) {
return super.skip(n);
}
int remaining = maxCharsFromTargetReader - charsRead;
if (markedAt >= 0) {
remaining = Math.min(remaining, readAheadLimit - (charsRead - markedAt));
}
if (remaining <= 0) {
return 0;
}
final long skipped = super.skip(Math.min(n, remaining));
charsRead += (int) skipped;
return skipped;
}
}
39 changes: 39 additions & 0 deletions src/test/java/org/apache/commons/io/input/BoundedReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import java.io.BufferedReader;
import java.io.File;
import java.io.FilterReader;
import java.io.IOException;
import java.io.LineNumberReader;
import java.io.Reader;
Expand Down Expand Up @@ -242,4 +243,42 @@ void testSkipTest() throws IOException {
assertEquals(-1, mr.read());
}
}

@Test
void testSkipDoesNotExceedBound() throws IOException {
try (BoundedReader mr = new BoundedReader(new StringReader("01234567890"), 3)) {
assertEquals(3, mr.skip(100));
assertEquals(-1, mr.read());
}
}

@Test
void testSkipDoesNotOverflowCharsRead() throws IOException {
try (BoundedReader mr = new BoundedReader(new StringReader("01234567890"), 3)) {
assertEquals(3, mr.skip(Long.MAX_VALUE));
assertEquals(-1, mr.read());
}
}

@Test
void testSkipUsesActualSkippedCount() throws IOException {
try (BoundedReader mr = new BoundedReader(new FilterReader(new StringReader("01234567890")) {
@Override
public long skip(final long n) throws IOException {
return super.skip(Math.min(n, 2));
}
}, 5)) {
assertEquals(2, mr.skip(5));
assertEquals('2', mr.read());
}
}

@Test
void testSkipRespectsReadAheadLimit() throws IOException {
try (BoundedReader mr = new BoundedReader(new StringReader("01234567890"), 10)) {
mr.mark(3);

@garydgregory garydgregory Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @sarankumarbaskar
I don't think you understand what mark is supposed to do. Please read the docs and see if there's a real issue once you fix these new tests.

@sarankumarbaskar sarankumarbaskar Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garydgregory Thanks, I see your point. Before changing this further, I want to clarify the intended behavior.

Should BoundedReader.skip(long) respect only maxCharsFromTargetReader, or should it also honor the active mark/readAheadLimit the same way read() currently does?

I can keep this PR limited to the clear issue — capping skip() to maxCharsFromTargetReader and updating charsRead with the actual skipped count — unless you think skip() should also mirror the readAheadLimit check.

@garydgregory garydgregory Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the contract of the mark and reset methods and notice the word "attempt". You're trying to turn override one feature (reading) with another (mark/reset).

Unless you can provide a strong argument that marking should act as a hard limit, I don't see a use case here.

@sarankumarbaskar sarankumarbaskar Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garydgregory Thanks, that clarifies it. I removed the mark/readAheadLimit-specific logic and test. The PR is now limited to skip(long) respecting maxCharsFromTargetReader and updating charsRead with the actual skipped count returned by the delegate.

assertEquals(3, mr.skip(100));
assertEquals(-1, mr.read());
}
}
}
Loading