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 Client and Server Transport Layers based on WebTransport #107

Closed

Conversation

TrustNoOneElse
Copy link
Contributor

@TrustNoOneElse TrustNoOneElse commented Sep 7, 2023

Overview

This pull request introduces two transport layers, one for the client and one for the server, built on top of the WebTransport API. These transport layers enable efficient and reliable communication between clients and servers over the web. Additionally, we provide a comprehensive example to showcase their usage.

Todos

  • Server Transport working (with h3 or wTransport)
  • Client Transport working
  • Light Chat Example
  • Documentation

Additional Notes

We could add as an optional Feature that the Client Webtransport runs in a Webworker. Since im not from the users which often implement games for browsers, im not sure how much sense it would make. Also since the concept is mostly pulling data from the update method which is called trough user code.

Happy for every feedback.

@TrustNoOneElse
Copy link
Contributor Author

Having some certs problem at the moment, 46 certificate unknown a tls handshake problem. Not really sure what broke or what i missed, because last week it was working xD So if anyone knows what could be wrong with the cert generation on the server side, i would be grateful. I will try this go tool for generating the cert later, maybe that will help. But yeah thats why there is currently no progress

@TrustNoOneElse
Copy link
Contributor Author

Finally good news, i got the local certs working again. But chrome somehow is dead on my system (it doesn't accept anymore flags or trusted certs), so i will instead modify the readme, to support firefox for windows. I hope thats okay for everyone, i will link a Linux version from a different repo for chrome.

- certificate generation reworked
- non blocking read for server
- non blocked read for client WIP
@TrustNoOneElse
Copy link
Contributor Author

image
Another good update, i finally managed, to have a non blocking client running, which can now send data to the server!
And also recieve!
image
Now i just need make the server also non blocking when its reading the datagram stream. Afterwards i can develop further the example and finally make clippy/format things and look other things up :D

@TrustNoOneElse TrustNoOneElse marked this pull request as ready for review October 2, 2023 13:17
@TrustNoOneElse
Copy link
Contributor Author

TrustNoOneElse commented Oct 2, 2023

Okay i tested a bit more and the write on the client side is also blocking when the server was shutdown forcefully. So i need to move this also to worker, i taking this back to draft until resolved

@TrustNoOneElse TrustNoOneElse marked this pull request as draft October 2, 2023 16:05
- Fix server too many lost clients
- Client now doesn't need async
@TrustNoOneElse TrustNoOneElse marked this pull request as ready for review October 2, 2023 19:05
@lucaspoffo lucaspoffo self-requested a review October 4, 2023 01:10
@UkoeHB
Copy link
Contributor

UkoeHB commented Oct 7, 2023

We could add as an optional Feature that the Client Webtransport runs in a Webworker. Since im not from the users which often implement games for browsers, im not sure how much sense it would make. Also since the concept is mostly pulling data from the update method which is called trough user code.

Have you done any benchmarks comparing web workers vs wasm bindgen futures? Web workers have a lot of overhead, so I'm not sure they are performant for a mainly IO use-case like this.

@TrustNoOneElse
Copy link
Contributor Author

Have you done any benchmarks comparing web workers vs wasm bindgen futures? Web workers have a lot of overhead, so I'm not sure they are performant for a mainly IO use-case like this.

I didn't find a way which would be able to handle the read outside of the web worker. The main problem was, that the await(JsFuture into Rust async) would hold the wasm ptr for as long as no message is there. This would mean that every next usage of the app would then throw with recursive use and the app would no longer be usable.

For the writing part i didn't test it further with worker, i had one running example but for the simple use case of a chat app it was hard to tell the difference and i wanted to avoid to give the worker more load by also writing and so maybe delay the reading depending on the load.

@5tr1k3r
Copy link

5tr1k3r commented Oct 13, 2023

Hi! Very nice work, hoping to see it merged soon! I spotted a consistent spelling mistake: receive should be spelled like so (and not recieve). I hope this is not bad tone to mention it here.

Copy link
Contributor

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Very nice :)

I'm not sure the worker is actually useful, but putting it in a feature is good.

renet_webtransport/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 1 to 2
use js_sys::Promise;
/// This module contains the bindings to the WebTransport API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use js_sys::Promise;
/// This module contains the bindings to the WebTransport API.
use js_sys::Promise;
/// This module contains the bindings to the WebTransport API.

Copy link
Contributor

@UkoeHB UkoeHB Oct 13, 2023

Choose a reason for hiding this comment

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

Or actually the shortcut should go after the comment.

TrustNoOneElse and others added 3 commits October 13, 2023 18:57
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Copy link

@5tr1k3r 5tr1k3r 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 sorry, I must have worded things poorly. I suggested the correct spelling now.

renet_webtransport/src/client.rs Outdated Show resolved Hide resolved
renet_webtransport/src/client.rs Outdated Show resolved Hide resolved
renet_webtransport/src/client.rs Outdated Show resolved Hide resolved
renet_webtransport/src/client.rs Outdated Show resolved Hide resolved
renet_webtransport/src/client.rs Outdated Show resolved Hide resolved
renet_webtransport_server/src/lib.rs Outdated Show resolved Hide resolved
renet_webtransport_server/src/lib.rs Outdated Show resolved Hide resolved
renet_webtransport_server/src/lib.rs Outdated Show resolved Hide resolved
renet_webtransport_server/src/lib.rs Outdated Show resolved Hide resolved
renet_webtransport_server/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Alex <noblemonitor@gmail.com>
@TrustNoOneElse
Copy link
Contributor Author

I'm sorry, I must have worded things poorly. I suggested the correct spelling now.

ah thanks, i guess my brain autocompleted the sentence and filtered out "and not" lol, thanks for the suggestions :D!

Copy link
Contributor

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Another review from me.

I now see two important issues with this PR.

  • Client ids are defined from an internal iterator. This means a server cannot directly map a pre-defined client list onto new connections (you would need a bespoke handshake after connecting to establish the client id mapping, which would be full of security challenges). With the netcode transport, client ids are assigned by the server when creating ConnectTokens, which allows the server to map 'internal user ids' onto 'renet client ids' up front. It also means the server doesn't have to re-map clients after a reconnect.
  • It seems that any client can connect to a server. Implementers must add a bespoke handshake on top of renet to validate new connections, which goes against the API expectation that you can safely send messages to clients once they are connected.

Both of these problems could be addressed by adopting the netcode ConnectToken workflow. Servers should issue WtConnectToken to clients. Then, the server should expect a valid WtConnectToken from connection requests, and should affiliate the client id embedded in that token with sent/received packets.

As a first-pass/proof-of-concept I would directly port ConnectToken logic into this PR, and then we can think about refinement. A big problem I have with the netcode protocol is it doesn't support reusing a ConnectToken for reconnects - the server needs to reissue tokens every time. It would be super nice to A) update the netcode protocol to enable token reuse (this is a project for me), B) for this webtransport PR to enable token reuse out of the box. EDIT: on further reflection, there are no good solutions for reconnects so I don't think the netcode protocol can be improved in this area.

pub fn update(&mut self, renet_server: &mut RenetServer) {
let mut clients_added = 0;

if let Ok(session) = self.connection_receiver.try_recv() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to drain all new sessions, not just poll for one session.

renet_webtransport/src/client.rs Outdated Show resolved Hide resolved
@TrustNoOneElse
Copy link
Contributor Author

Another review from me.

I now see two important issues with this PR.

  • Client ids are defined from an internal iterator. This means a server cannot directly map a pre-defined client list onto new connections (you would need a bespoke handshake after connecting to establish the client id mapping, which would be full of security challenges). With the netcode transport, client ids are assigned by the server when creating ConnectTokens, which allows the server to map 'internal user ids' onto 'renet client ids' up front. It also means the server doesn't have to re-map clients after a reconnect.
  • It seems that any client can connect to a server. Implementers must add a bespoke handshake on top of renet to validate new connections, which goes against the API expectation that you can safely send messages to clients once they are connected.

Both of these problems could be addressed by adopting the netcode ConnectToken workflow. Servers should issue WtConnectToken to clients. Then, the server should expect a valid WtConnectToken from connection requests, and should affiliate the client id embedded in that token with sent/received packets.

As a first-pass/proof-of-concept I would directly port ConnectToken logic into this PR, and then we can think about refinement. A big problem I have with the netcode protocol is it doesn't support reusing a ConnectToken for reconnects - the server needs to reissue tokens every time. It would be super nice to A) update the netcode protocol to enable token reuse (this is a project for me), B) for this webtransport PR to enable token reuse out of the box. EDIT: on further reflection, there are no good solutions for reconnects so I don't think the netcode protocol can be improved in this area.

Okay after a lot of reading i get what this is about. Im not sure how complex it will be to get this mechanism into this. Thanks for the finding and i can understand the concern that it is not secure enough.

@UkoeHB
Copy link
Contributor

UkoeHB commented Dec 15, 2023

Im not sure how complex it will be to get this mechanism into this.

I would look for the lowest common denominator between renetcode and webtransport, then port all the renetcode logic on top of webtransport. Renetcode has a lot of connection management logic that is quite useful for games.

It's possible that renetcode could be refactored into two crates: the netcode protocol pieces, and the integration with UDP sockets. Then the netcode protocol piece can be reused for the webtransport implementation. I haven't looked deeply into this, so I don't fully understand how much webtransport differs from UDP, or how much of the netcode protocol is not needed when using webtransport.

@TrustNoOneElse
Copy link
Contributor Author

I would look for the lowest common denominator between renetcode and webtransport, then port all the renetcode logic on top of webtransport. Renetcode has a lot of connection management logic that is quite useful for games.

i will try my best, thanks for the tips! I guess we could refactor the token out like you say, @lucaspoffo, do you have any preferences on that?

@UkoeHB
Copy link
Contributor

UkoeHB commented Feb 4, 2024

@TrustNoOneElse The issues I mentioned would be fixed by migrating your implementation to use the TransportSocket trait from #145, which should be very easy. Let's hope poffo merges that.

@UkoeHB
Copy link
Contributor

UkoeHB commented Apr 8, 2024

@TrustNoOneElse can you give more detailed instructions for running the client example? I am having a lot of pain trying to run it. You reference ../pkg in package.json but idk what's supposed to be in there.

@TrustNoOneElse
Copy link
Contributor Author

@TrustNoOneElse can you give more detailed instructions for running the client example? I am having a lot of pain trying to run it. You reference ../pkg in package.json but idk what's supposed to be in there.

Was answered on discord, but for everyone else wondering you need to build the example rust library with "wasm-pack build" :)

@UkoeHB
Copy link
Contributor

UkoeHB commented Apr 20, 2024

This is now implemented in renet2, using this PR as a base.

@TrustNoOneElse
Copy link
Contributor Author

Thanks @UkoeHB with that there is no reason to hold this PR any longer open :)

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