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.

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

[HttpClient] [EventSourceHttpClient] Fix consuming SSEs with \r\n separator #54242

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

fancyweb
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

\r\n separator never worked because it splits on every line because of [\r\n].

@carsonbot

This comment was marked as outdated.

@carsonbot carsonbot changed the title [HttpClient][EventSourceHttpClient] Fix consuming SSEs with \r\n separator [HttpClient] [EventSourceHttpClient] Fix consuming SSEs with \r\n separator Mar 11, 2024
@fancyweb fancyweb changed the base branch from 7.1 to 5.4 March 11, 2024 18:03
@fancyweb fancyweb modified the milestones: 7.1, 5.4 Mar 11, 2024
@fancyweb
Copy link
Contributor Author

fancyweb commented Mar 11, 2024

There's another regex line 147 but I didn't modify it for safety (since we're on 5.4). It's untested, so its goal is unclear. Judging from the pattern, the intention might be to yield SSE comments (line starting with a colon) as DataChunk. If that's the case, then it's not working at all and it's another bug 😅

cc @nicolas-grekas @dunglas @soyuka since you worked on the initial implementation and you might remember something 🙏

Copy link
Contributor

@soyuka soyuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, when the line starts with a colon we need to skip see https://html.spec.whatwg.org/multipage/server-sent-events.html#event-stream-interpretation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 13, 2024

when the line starts with a colon we need to skip

yep, that's what we do on L147

@fancyweb
Copy link
Contributor Author

yep, that's what we do on L147

It's not working as expected. I'll propose a patch, but on 7.1 because it's going to change the behavior heavily. Is that OK for you?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 13, 2024

on 7.1 because it's going to change the behavior heavily

Not sure: if we don't follow the spec, that's a bug and we need to fix it in 5.4.

@fancyweb
Copy link
Contributor Author

We don't follow the spec but we've been yielding comments as SSE for more than 3 years 😅

@nicolas-grekas
Copy link
Member

It doesn't matter, we don't want to preserve BC for a private buggy protocol :)

@fabpot
Copy link
Member

fabpot commented Apr 5, 2024

Thank you @fancyweb.

@fabpot fabpot merged commit cfb999e into symfony:5.4 Apr 5, 2024
7 of 12 checks passed
@fancyweb fancyweb deleted the hc/fix-event-source-sep-54 branch April 5, 2024 06:58
This was referenced Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants