Fill the lookahead buffer on short reads from a chunked source#625
Conversation
A Reader that delivers data in chunks makes the buffered read return early, so the delimiter lookahead was compared against a partially filled buffer and multi-character delimiters were missed.
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Hello @saleemno1
Please see my questions and comments.
| @Test | ||
| void testParseWithDelimiterStringFromChunkedReader() throws IOException { | ||
| // A reader that hands out one character at a time and never reports itself ready, like a socket or pipe. | ||
| final Reader chunked = new Reader() { |
There was a problem hiding this comment.
If this is the same as the behavior of the ChunkedReader in the other test, then refactor for reuse.
There was a problem hiding this comment.
Right, they were the same thing. Pulled it out into a package-private ChunkedReader in the test tree so both this test and ExtendedBufferedReaderTest use it.
| /** | ||
| * A reader that returns one character per call and never reports itself ready, like a socket or pipe that delivers data in chunks. | ||
| */ | ||
| private static final class ChunkedReader extends java.io.Reader { |
There was a problem hiding this comment.
Would it be simpler to extend StringReader and override those methods?
There was a problem hiding this comment.
Done. It's now a small ChunkedReader extends StringReader that overrides read(char[], int, int) to hand back one char and ready() to return false, which is a lot less code than the raw Reader.
| final int len = super.read(buf, offset, length); | ||
| int len = super.read(buf, offset, length); | ||
| // The underlying buffered reader stops early once the source reports it is not ready, so a stream that delivers data in chunks (a socket or a pipe) | ||
| // yields a short read. Callers match multi-character sequences against this buffer, so keep reading until it is full or the source is exhausted. |
There was a problem hiding this comment.
Isn't there still a risk that even filling to the end of the buffer breaks up multi-character sequences?
There was a problem hiding this comment.
No, because the callers size the array to the exact sequence they're matching. Lexer allocates delimiterBuf as delimiter.length - 1 and escapeDelimiterBuf as 2 * delimiter.length - 1, so once the array is full it holds the whole tail of the delimiter (or escaped delimiter), never a piece of it. Same for CSVFormat.printWithEscapes, which peeks delimiter.length - 1.
The only way the loop exits short now is EOF, and then the remaining characters genuinely don't exist, so no complete sequence can be there to miss. Before the change the loop could exit short just because the source wasn't ready yet, which is the case that broke.
garydgregory
changed the title
2 participants
Thanks for maintaining this!
ExtendedBufferedReader.peek(char[])andread(char[], int, int)inherit a buffered read that returns as soon as the source reports it is not ready, so aReaderbacked by a socket or a pipe hands back a short read and the delimiter lookahead is matched against a half-filled buffer.Two things go wrong once the buffer is short:
Lexer.isDelimiter/isEscapeDelimitercompare every slot, so the still-zeroed tail makes a multi-character delimiter that is really present look like ordinary text. Parsinga[|]bwith delimiter[|]yields one fielda[|]bfrom a chunked reader and two fieldsa,bfrom aStringReaderfor the same bytes, which is a parser differential if the split feeds any filtering or routing decision.CSVFormat.printWithEscapes(Reader, Appendable)builds its lookahead the same way, so printing the single valuex[|]yfrom a chunked reader writes the delimiter unescaped and the record reads back as two fields.Both call sites share the same reader, so the loop lives there rather than in each caller. Added a regression test at the reader level and one at the parser level; both fail without the change.
mvnis green including the coverage gate.