Fix parsing stream with first token size >= parser's chunk size - #45
Conversation
84c07ed to
eab3a71
Compare
n-peugnet
changed the title
n-peugnet
changed the title
n-peugnet
changed the title
2e9b9e9 to
99801b8
Compare
| defer stream.Close() | ||
|
|
||
| require.True(t, stream.IsValid()) | ||
| assert.True(t, stream.CurrentToken().IsKeyword()) |
There was a problem hiding this comment.
please use require instead of assert
99801b8 to
fda5579
Compare
|
|
||
| // TestIssue44 The parser stops parsing the stream after the first token if | ||
| // its size is very close to the size of the parser's buffer. | ||
| func TestIssue44(t *testing.T) { |
There was a problem hiding this comment.
At fisrt, thanks for the PR. The fix itself looks correct.
One small thing: I ran TestIssue44 andTestIssue44NextToken on master and they pass there too - so they don't actually catch the bug yet. The reason is the input is exactly 43 bytes with bufferSize = 43, so preload() reads everything in one shot and the first parse() already emits both tokens before any GoNext(). The bug only shows up when the reader has more data than fits in one buffer
Could you tweak the test data so it fails without the fix? Something like bufSize=20 with "this_token_is_exactly_40_characters_long 67 next_keyword_here 99" could reproduce it - 1 token instead of 4 on master, all 4 with your fix
There was a problem hiding this comment.
Ah yes, this is because you added the fast path for ASCII, and ensureBytes(4) is not always called anymore. I will updates the tests and commit messages
There was a problem hiding this comment.
Just made the changes and updated the commits and PR description and title
When (first token size) >= (parser's chunk size), the next chunks are loaded during the parsing of the first token. This triggers the end of the parsing loop, leaving the frist token as the only one parsed, and thus having its "next" field set to nil. This in turn, lead Stream.GoNext() to detect the end of the stream. To fix this, Stream.GoNext() now starts by checking the next field of the current token, parsing the current chunk if needed, before deciding if the end of the stream is reached. Fixes bzick#44
NextToken() also assumed that the next token was always parsed, which is not the case. Add a new ensureParseNext() function that can be used in both GoNext() and NextToken(), and that makes sure the next token is parsed if the end of the stream has not been reached yet.
fda5579 to
2a49f87
Compare
n-peugnet
changed the title
n-peugnet
changed the title
2 participants
When (first token size) >= (parser's chunk size), the next chunks are
loaded during the parsing of the first token. This triggers the end of
the parsing loop, leaving the first token as the only one parsed, and
thus having its "next" field set to nil. This in turn, lead
Stream.GoNext() to detect the end of the stream.
To fix this, Stream.GoNext() now starts by checking the next field of
the current token, parsing the current chunk if needed, before deciding
if the end of the stream is reached.
NextToken() also assumed that the next token was always parsed, which is
not the case. Add a new ensureParseNext() function that can be used in
both GoNext() and NextToken(), and that makes sure the next token is
parsed if the end of the stream has not been reached yet.
Tests have been added to prevent regressions.
Fixes #44