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

Add WebSocket support for RemoteHttpPlugin #16403

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

Conversation

villor
Copy link
Contributor

@villor villor commented Nov 16, 2024

Objective

BRP currently has a HTTP transport implementation, including watching requests using SSE (server-sent events).

While SSE can be great, especially when combined with HTTP/2, it has a significant limitation when used with HTTP/1.1. The browser will limit the amount of concurrent requests to a very low number.

There are other nice options today like WebTransport, but all of them (AFAIK) suffer from being limited to secure (HTTPS) connections only. Same thing applies to SSE over HTTP/2.

To keep the developer-friendly experience of not needing a certificate, while also allowing more concurrent watches, WebSocket seems like the only viable option.

Solution

  • Implemented WebSocket upgrades for RemoteHttpPlugin using tungstenite. This way the connection uses the same endpoint (host/port) as the HTTP requests.
  • Requests are sent as Text messages serialized to JSON, following the JSON RPC 2.0 format of BRP.
  • Responses are sent back in undefined order. The client is responsible for keeping track by using the id field in JSON RPC 2.
  • Batched requests work the same as with HTTP. An array can be sent in, and an array with all the responses will be sent back once they are all processed.
  • Watchers will keep sending responses with a matching ID.
    • Limitation: There is no way to cancel a watching request. With SSE this was not a problem since closing the connection worked. For a continous streaming transport like WebSocket, we need a way to unwatch requests. This is handled in Add BRP watch_id and bevy/unwatch #16407
  • Feature flagged it behind remote_websocket (non-default)
  • Extra: Made the dependencies used by the http feature optional and only included when the feature is enabled.

Testing

  • Ran the server example and used a web based tool to test both complete and watched requests, as well as batched requests.
  • Ran the server/client examples to ensure HTTP still works.

To test it

Run the server example with WebSocket enabled:

cargo run --example server --features remote_websocket

Follow-up

Reduce dependency footprint, potentially by:

  • Replacing hyper-tungstenite with custom implementation. It is a tiny library, but adds a (non-optional) tokio dependency which should not be needed.
  • See if the uses of futures-util can be replaced with smol alternatives.

Copy link
Contributor

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

@BenjaminBrienen BenjaminBrienen added C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Networking Sending data between clients, servers and machines labels Nov 17, 2024
@alice-i-cecile alice-i-cecile added the A-Dev-Tools Tools used to debug Bevy applications. label Nov 17, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this as off-by-default :) Seems generally useful. Agree on cutting out the tokio dep later though.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 17, 2024
Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 17, 2024
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Nov 17, 2024
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. A-Networking Sending data between clients, servers and machines C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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