test(ssestream): discard incomplete events at EOF#697
Conversation
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Thank you very much for the focused change and the clear regression tests — I appreciate you taking the time to investigate this edge case.
I found two blocking issues in the proposed EOF dispatch path:
- The behavior conflicts with the normative SSE parsing algorithm. The WHATWG HTML Standard says that once EOF is reached, pending data is discarded and an incomplete event is not dispatched; it also gives an explicit example noting that the last event still needs a blank line. See https://html.spec.whatwg.org/multipage/server-sent-events.html#event-stream-interpretation.
- The
event != ""condition emits type-only blocks with empty data. I reproduced this on the PR head withevent: updateat EOF: the decoder returned{Type:update Data:[]}, andStream.Next()then failed withunexpected end of JSON input. That is the same empty-data failure mode recently fixed by #621.
The existing go test ./packages/ssestream suite passes on the branch, but focused decoder and typed-stream regressions for the type-only EOF case both fail. I’ve left inline comments with the details.
| return false | ||
| } | ||
|
|
||
| if data.Len() > 0 || event != "" { |
There was a problem hiding this comment.
This EOF dispatch is contrary to the SSE parsing algorithm: the WHATWG specification says pending data must be discarded at EOF and that an incomplete event is not dispatched. Separately, || event != "" reintroduces an empty-data failure: with event: update followed by EOF, this returns {Type: "update", Data: []}, and a typed Stream terminates with unexpected end of JSON input. Please keep EOF from dispatching incomplete blocks; if non-standard tolerance is intentionally desired, it needs an explicit compatibility decision and must at least never dispatch when data.Len() == 0.
| Header: http.Header{ | ||
| "Content-Type": []string{"text/event-stream"}, | ||
| }, | ||
| Body: io.NopCloser(strings.NewReader("event: update\ndata: hello")), |
There was a problem hiding this comment.
This fixture encodes behavior that the SSE standard explicitly rejects: EOF is not an event delimiter, and the pending event:/data: block must be discarded without a final blank line. The compliant fixture would end with \n\n; without that delimiter, decoder.Next() should return false.
d7a4dea to
c0410cb
Compare
c0410cb to
f0ec517
Compare
|
Thanks for the detailed review. I’ve removed the EOF dispatch behavior and updated the tests to assert the standards-compliant behavior: buffered events without a terminating blank line are discarded at EOF, including type-only events with no data. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback and for the focused regression coverage. I re-reviewed the updated change and everything looks good now. Approved!
Why
EOF is not an SSE event delimiter. When a stream ends without a terminating blank line, the pending event must be discarded. This PR adds regression coverage for that behavior, including type-only blocks that must not produce empty data.
What changed
This PR adds test coverage only; the runtime behavior is already standards-compliant.
Validation
go test -count=1 -race ./packages/ssestream