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

docs: circumvent oddities with SSE and compression #2585

Closed
wants to merge 2 commits into from

Conversation

BenJeau
Copy link

@BenJeau BenJeau commented Feb 11, 2024

Motivation

I was working with SSEs for my personal project and wasn't able to figure out why sometimes I received some events, only if the events were big. Thought it was something with Chrome, Firefox, or Postman. Was able to view the full response with curl but not within the rest.

After trying different things, I saw #2034 (reply in thread) and it fixed my issue.

Solution

The solution is to either remove the compression layer itself or to add an exception for SSEs. I've added a note on that in the docs related to SSEs

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

It should be pretty straight forward to fix in tower-http. Would you be up for giving it a shot? There is an issue for it tower-rs/tower-http#420. Should be able to update DefaultPredicate to include event streams.

@BenJeau
Copy link
Author

BenJeau commented Feb 11, 2024

That seems reasonable to add it to DefaultPredicate, opened a PR on tower-http's side tower-rs/tower-http#465

@BenJeau
Copy link
Author

BenJeau commented Feb 13, 2024

With the code merged from tower-http's side, I still think there should be a mention about compression in the Axum's SSE docs - maybe not specifically about tower-http - in case someone uses compression but not with the DefaultPredicate.

@davidpdrsn what do you think? If you think otherwise, feel free to close this PR

@stevenengler
Copy link

Just wanted to mention that if you still want compression on your SSE streams rather than disabling it altogether, you can write your own futures_util::stream::Stream that wraps the axum::body::BodyDataStream and compresses and flushes SSE stream events. This avoids the buffering issues with tower's compression middleware. I wrote a small example at #2728 in the hope that someone passing through might find it helpful.

@paolobarbolini
Copy link
Contributor

I guess since tower-rs/tower-http#465 was merged this is not necessary anymore?

Just wanted to mention that if you still want compression on your SSE streams rather than disabling it altogether, you can write your own futures_util::stream::Stream that wraps the axum::body::BodyDataStream and compresses and flushes SSE stream events.

This would still be cool to have, but it should be an issue in tower-http

@jplatte
Copy link
Member

jplatte commented Sep 28, 2024

I think it's probably not that important to have a note about this sort of problem in the docs now that the one well-known instance of it is fixed. Also seems like the author was not interested in updating it to explain to the underlying problem after the tower-http instance was fixed.

@jplatte jplatte closed this Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants