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

Streaming bevy/get and bevy/list with HTTP SSE #15608

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

Conversation

LiamGallagher737
Copy link
Contributor

@LiamGallagher737 LiamGallagher737 commented Oct 3, 2024

Objective

Add a way to stream BRP requests when the data changes.

Solution

BRP Side (reusable for other transports)

Add a new "update checker" handler alongside a normal method handler that works a lot like a run condition. Streaming requests are stored in a resource, every request in it is checked every frame for updates and the main handler is run if the update condition returns true. The new response is sent over the same channel as normal. Transports decide how to decide if requests should be streamed or not as some like web sockets will always want to stream and others may want to be a hybrid (like I did with HTTP).

HTTP Side

If a request comes in with +stream in the method, the transport will invoke the method with streaming enabled. It will then reply with text/event-stream content-type (Server sent events) and keep the connection open for more responses.

Testing

I tested with the podman HTTP client. This client has good support for SSE's if you want to test it too.

Parts I want some opinions on

For selecting streaming in HTTP I chose to add a +stream suffix to the end kind of like content-type headers. A get would be bevy/get+stream. Anything after a '+' is then ignored when selecting a handler. I chose this as I don't think the functionality is different enough for a new method name but I also don't want to add to the body params.

Future work

  • Currently the full response is resent, it would be nice to only send whats changed but for now I want to keep things as simple as I can keep them.
  • The bevy/query method would also benefit from this but that condition will be quite complex so I will leave that to later.

@JMS55
Copy link
Contributor

JMS55 commented Oct 3, 2024

Really not sure how I feel about SSE vs a proper streaming protocol like QUIC or WebTransport...

@LiamGallagher737
Copy link
Contributor Author

Really not sure how I feel about SSE vs a proper streaming protocol like QUIC or WebTransport...

I chose it just due to being easy to implement with the existing HTTP transport. I am happy to swap it for something else as it's the BRP part I care about, the SSE is just to have something to show it works.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Dev-Tools Tools used to debug Bevy applications. S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 3, 2024
@LiamGallagher737
Copy link
Contributor Author

After a quick look at the wtransport crate, it is defiantly something I'd like to have a go at integrating. Until then I think the SSE serves as a good example of how to use the streaming.

@LiamGallagher737
Copy link
Contributor Author

@notmd I'd also appreciate a review from you since you have been working on streaming stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Dev-Tools Tools used to debug Bevy applications. C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants