Skip to content

Don't compress on server sent events#2871

Merged
Kludex merged 4 commits intomasterfrom
dont-compress-on-sse
Feb 22, 2025
Merged

Don't compress on server sent events#2871
Kludex merged 4 commits intomasterfrom
dont-compress-on-sse

Conversation

@Kludex
Copy link
Owner

@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
Owner 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
Owner 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
Owner Author

Kludex commented Feb 22, 2025

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

@michi42
Copy link

michi42 commented Oct 31, 2025

Would it be possible to make the behavior configurable (excluded types, and whether to flush after each chunk for streaming responses) as parameters when creating the middleware?

The behavior proposed in #2753 makes a lot of sense if you expect relatively large SSE messages - we have e.g. an use case where we stream data for live charts over SSE, updates can reach 10-100 KB in size, which would profit significantly from compression.

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.

3 participants

Comments