fix: buffer progress chunks so a split event terminator doesn't merge events - #40
Open
emaglic-xa-eg wants to merge 1 commit into
Open
fix: buffer progress chunks so a split event terminator doesn't merge events#40emaglic-xa-eg wants to merge 1 commit into
emaglic-xa-eg wants to merge 1 commit into
Conversation
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.
Fixes #38.
Problem
When a network chunk boundary lands inside an event's
\n\nterminator, two events get concatenated into one. The merged payload is invalid for consumers that parsedata(for exampleJSON.parse), and when the trailing event is the stream's final one, the preceding event is dropped with no error at all.Cause
onStreamProgresssplit each progress slice independently:A progress slice can end partway through a terminator, so neither slice contains the full
\n\n. The boundary is never recognized, the first newline is buffered as event content, and the next event is appended before the flush happens, so both events land in one chunk and merge.Fix
Buffer the raw text across progress slices and only emit once a full terminator is present. Anything after the last complete terminator stays buffered for the next slice, so a split terminator just means "keep buffering" instead of "merge":
I also reset the buffer on the
Sentevent. AretryWhenreconnection skipsonStreamCompleted, which is the only place the buffer was cleared, so without this a partial event from a dropped connection would merge into the first event of the reconnected stream.Sentfires on every (re)connection, so it is a reliable per-connection reset point.The old
parseEventDatahelper was the only caller of the split path and is now unused, so I removed it.Repro
Dependency-free, runs with
node repro.mjs. It uses the library's realparseEventChunk/parseChunkLineand only swapsonStreamProgress(current vs this PR), fed the same cumulativepartialTextsnapshots the browser delivers, with a boundary inside the terminator:repro.mjs
Output:
Validation
npm run build:prodpasses.Tests
The library doesn't have a test harness wired up right now (no specs, and karma/jasmine aren't in the dev dependencies), so I kept this PR to the fix plus the standalone repro above rather than adding a test framework unprompted. Happy to follow up with a spec, or to set up Karma/Jasmine in a separate PR, if you'd like that.