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

Make SSE less dependent on tokio #3154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

nanoqsh
Copy link

@nanoqsh nanoqsh commented Jan 7, 2025

Motivation

Currently, to return an SSE response the tokio feature is required, even though Tokio runtime is only needed for timeouts to maintain keep-alive connection. This limitation restricts the use of SSE with other runtimes.

Solution

The KeepAliveStream now wraps a Stream<Item = Result<Event, _>> and implements Stream<Item = Result<Event, _>> itself to maintain the keep-alive connection. This feature is optional and users can choose not to use keep-alive or implement their own wrapper if needed.

The keep-alive message itself is now represented by the Event type, making it more convenient to wrap a user-defined Stream<Item = Result<Event, _>>. If a user wants to implement their own wrapper, they can use Event::DEFAULT_KEEP_ALIVE, which is a cheap to clone default keep-alive message.

Impl #3153

@nanoqsh nanoqsh mentioned this pull request Jan 7, 2025
1 task
@nanoqsh nanoqsh changed the title Update Sse::keep_alive Make SSE less dependent on tokio Jan 7, 2025
@mladedav
Copy link
Collaborator

mladedav commented Jan 7, 2025

This makes sense to me as a whole, there are just two potentially breaking things I see

  • people using keep alive will have different type parameter in S so if they had to name it for any reason, their code will no longer compile
  • calling keep_alive multiple time used to be idempotent and only the last one had any effect. Now it will compose and lead to using the lowest supplied value with possibly some spurious events.

So I don't think we can merge this before 0.9. But in the meantime we could just keep the old keep_alive behind the feature gate and switch to this implementation later.

@nanoqsh
Copy link
Author

nanoqsh commented Jan 7, 2025

potentially breaking

@mladedav Yes, I didn't try to keep backwards compatibility completely. I don't think it's possible with a good design. I didn't want to change anything drastically, but yes, it would be reasonable to wait for 0.9

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