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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

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