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

Don't compress on server sent events #2871

Merged
merged 4 commits into from
Feb 22, 2025
Merged

Don't compress on server sent events #2871

merged 4 commits into from
Feb 22, 2025

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Feb 16, 2025

@vin vin mentioned this pull request Feb 17, 2025
3 tasks
@vin
Copy link

vin commented Feb 17, 2025

Some things that still appear a little off to me:

  1. we do write without flush followed by an immediate read, so the read value may (and often is) empty. Since this is intentional, I think it deserves a code comment and arguably a different behavior: does it make sense to await self.send(message) when the message body is empty?
  2. The documentation currently states The middleware will handle both standard and streaming responses. - I think this deserves to include some more detail about when it will be disabled, and when streaming responses may be buffered resulting in potential delays.

@Kludex
Copy link
Member Author

Kludex commented Feb 17, 2025

The documentation currently states The middleware will handle both standard and streaming responses. - I think this deserves to include some more detail about when it will be disabled, and when streaming responses may be buffered resulting in potential delays.

We can add more information to the docs, yep.

we do write without flush followed by an immediate read, so the read value may (and often is) empty. Since this is intentional, I think it deserves a code comment and arguably a different behavior: does it make sense to await self.send(message) when the message body is empty?

I haven't consider the option of not sending empty messages. From the client point of view, nothing changes, since the server will not send anything anyway... If you have a middleware, it may help to know this.


As a first step, I think we need to add more details to the docs. Not much, just point out what happens on streaming.

@Kludex
Copy link
Member Author

Kludex commented Feb 18, 2025

It seems there are more content types that can be excluded, according to Ameobea/rocket_async_compression@64a984d

@Kludex Kludex merged commit a9a8dab into master Feb 22, 2025
5 checks passed
@Kludex Kludex deleted the dont-compress-on-sse branch February 22, 2025 12:54
@Kludex
Copy link
Member Author

Kludex commented Feb 22, 2025

Happy to add more content-types on the constant if asked for.

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.

2 participants