Skip to content
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

Jump to bottom

[ch83910] Rewrite decoder to fix some decoding bugs #3

Merged
merged 17 commits into from
Sep 3, 2020
Merged

[ch83910] Rewrite decoder to fix some decoding bugs #3

merged 17 commits into from
Sep 3, 2020

Conversation

samstokes
Copy link
Contributor

This fixes a few bugs and places we weren't quite compliant with the spec:

  • the handling for lines split across chunks wasn't quite right and could drop lines if that happened more than once in a stream.
  • we were stripping all leading spaces from field values, instead of just the first leading space.
  • multiple values for the same field would overwrite (now they concatenate instead)
  • a line without a colon was treated as an error (now it treats the whole line as the field name, with an empty field value)

Closes #2 (includes commits from that PR, which included tests that discovered some of these bugs).

hroederld and others added 16 commits July 20, 2020 09:53
@hroederld
add split chunk test
1181f84
@hroederld
add multiple data lines test
c9a1d32
@samstokes
fix formatting and add a NotReady so errors show up in CI
5b7bd27
@samstokes
Rewrite chunk-to-line decoding to handle line terminators _within_ chunk
a853cc0
@samstokes
tweak test delay handling, add another test case
c9fdd78
@samstokes
Append subsequent values for the same field instead of replacing
bb4d202
@samstokes
remove unnecessary mut
0908167
@samstokes
clean up trace logging a bit
1cc8cae
@samstokes
handle incomplete chunks in middle of stream
685146b
@samstokes
add env_logger in example script and mention logging in DEVELOPING.md
6f30d2f
@samstokes
get rid of some unwrap() for readability
75fb7af
@samstokes
remove placeholder test
0a932c3
@samstokes
less confusing ordering
cee673e
@samstokes
only strip a single initial space
98044c4
@samstokes
handle lines with no colons correctly
8b2dd42
@samstokes
jiggle around test cases instead of having one gigantic test function
2a1a2a8
@samstokes samstokes merged commit c420b50 into master Sep 3, 2020
@samstokes samstokes deleted the samstokes/ch83910/fix-whitespace-split branch September 3, 2020 23:55
@samstokes samstokes mentioned this pull request Mar 6, 2021
Merged
samstokes added a commit that referenced this pull request Mar 8, 2021
@samstokes
[ch88739] Fix decoding of multiple events in a single chunk (#6)
f8b170a 
Rewrite the SSE decoder again to fix a nasty bug that seemed to be occurring intermittently during IH tests (and in any case was reproducible). If two events occurred within a single chunk, we would drop one, which is unfortunate. This may have been introduced during the last rewrite (#3).

I also tried hard to simplify down the logic and remove some conditionals that didn't need to be there, so hopefully the result is a little more readable than what came before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hroederld hroederld hroederld approved these changes

@gwhelanLD gwhelanLD Awaiting requested review from gwhelanLD

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samstokes @hroederld