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

feat: add SSE compression exclusion within default predicate #465

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

BenJeau
Copy link
Contributor

@BenJeau BenJeau commented Feb 11, 2024

Motivation

Discussed in this Github discussion from Axum's repo:

And had opened a docs PR within Axum's repo to add a note about compression for SSEs:

If not careful or properly handled, general compression with SSE will result in missing some or all events.

Solution

Exclude SSEs from being compressed by checking the content type of text/event-stream within the DefaultPredicate

@FrankReh
Copy link

Could the problem have been that the compression buffer isn't flushed for the SSE case?

@jplatte jplatte merged commit ad25019 into tower-rs:main Feb 12, 2024
11 checks passed
@BenJeau
Copy link
Contributor Author

BenJeau commented Feb 13, 2024

Could the problem have been that the compression buffer isn't flushed for the SSE case?

Potentially, I didn't had the time to investigate but saw that disabling compression resolved all my issues.


I can't close the linked issue #420 - if someone could, it would be great to cleanup the issues

@jplatte
Copy link
Collaborator

jplatte commented Feb 13, 2024

I'll try to remember to close it after the fix has shipped in a crates.io release.

@ekzhang
Copy link

ekzhang commented Feb 14, 2024

Thanks for fixing this!

Besides text/event-stream, another candidate content type that involves dynamically pushing data is multipart/x-mixed-replace -- which is used sometimes in image tags: https://wiki.tcl-lang.org/page/multipart%2Fx-mixed-replace#:~:text=multipart%2Fx%2Dmixed%2Dreplace%20is%20an%20HTTP%20content%20type,content%20to%20the%20web%20browser.

(I think that one is a lot less common though)

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.

4 participants