Connection and Disconnection events

[connorcarpenter]

Hey there! This PR is to introduce a new public method on the Server impl, "get_event_receiver()" which optionally sets up a channel that will emit connection & disconnection events.

Idk if this is something you're interested in adding to the API, but it became necessary for my own project and maybe someone else can take advantage of it. Please give me some recommendations as I'm still learning Rust, totally down for a refactor. This is my first Rust PR!

oh btw, this crate rocks!

EDIT: You may have seen me open & close a previous PR, I just wanted to squash my commits together :+1:

[kyren]

Hey, thanks so much for taking a stab at this! Several people have asked for a similar API, so I know this is probably pretty important.

If I were to go with this strategy for connection events, there's a couple of pretty minor things in the PR that I'd want to address, but overall it's not far off from a sensible implementation of that strategy.

However, I'm not sure I want to go with this strategy for connection events at all. I think you quite sensibly looked at the implementation of Server, saw that it gives back a handle type for session setup, and mimicked that strategy for connection / disconnection events. The issue is that I actually really don't like that part of the API, and I don't think we should try to mimic it.

I think actually at the same time an API for this is added, Server should be refactored so that SessionEndpoint is not even necessary. The API I'm imagining is that Server::recv is renamed to something like, well Server::recv_event or maybe just Server::process, something to disconnect it with sending messages, and instead it returns all events, connection establishment, disconnection, timeout etc and incoming messages. I also think that the SessionEndpoint should be folded into Server and so when operating the server you should basically select! on 1) receiving an event from the Server and handling it, 2) receiving from whatever tells you when there are outgoing messages that need to be sent, and 3) receiving connection requests from something connected to a web server.

Or, maybe it should look like that. The neat thing about SessionEndpoint, and really the reason it exists, is that it allows you to send a response to the connecting client without waiting on the Server at all. The problem is that it's just too magical, what if you need to know somehow about the state of the underlying channel, what if, when the channel is full, you instead need to send back an error response to the connecting client and have them do exponential backoff?

I need to think about this some more, but I think SessionEndpoint shouldn't even have an implicit mpsc channel, the user should be forced to do that themselves to decide how best to do it. Server::accept_session would instead be public and the user would have to accept the sessions themselves. I know an API like I'm proposing makes using the crate slightly more difficult, but I'm not trying to make a crate that's easy to get started with as much as I am making a crate that has as few opinions as possible. If you're trying to make something really bullet-proof, these little hidden decisions can be killer.

So anyway I'm DEFINITELY not asking you to do all that, rather I'm just explaining why I might not be able to get around to this PR very soon or it might become superseded by other changes. I'm gonna let myself think about this a bit more and try to address it, either by accepting this strategy and walking you through the minor changes, or by deciding on a different strategy altogether.

Sorry for making this more complicated than you were probably hoping for, but whatever the outcome of this PR is the API will eventually have the ability to receive connection events, so you shouldn't have to use your own fork indefinitely.

[kyren]

I should probably also explain the other reason that this API doesn't exist yet and I personally haven't really needed it so far. Here's what I do:

• When clients connect over WebRTC, they send the first message to connect to the server
• When the server receives a message from a new client that it doesn't have an established connection for, it expects the first message to be a connection init. If it's not then that's an error, if it is then this establishes the connection at the level of the game server.
• The game server constantly sends messages, if it ever notices a failure due to disconnection when sending a message, then that client becomes disconnected.

This way, there's not really a need for connection events at all. Now I'm not saying that such an API shouldn't exist, I'd still like to add it since it can be useful for other reasons or for different strategies, I'm just saying why I haven't needed it so far.

[connorcarpenter]

Hey Kyren, thanks for the writeup! Yeah that totally makes sense, and I agree that, since it's the first of it's kind, this crate should have a focus on being efficient & opinionated over usability. That way it can be the "engine" for various other strategies / APIs.

Now I'm curious if I can adopt the same patterns you've been using to catch connections/disconnections. While I think I can implement your connection detection strategy, I can imagine periods in my game where the server wouldn't constantly be sending messages, and I'd like to catch a disconnect quickly. I believe the server already sends heartbeat packets to the client to maintain the WebRTC connection, and it feels weird to be manually sending my own heartbeat packets on top of that to detect disconnects.

No rush at all, I'm good deving with my own fork for now. Thanks for looking into / thinking about this :+1:

[kyren]

Now I'm curious if I can adopt the same patterns you've been using to catch connections/disconnections. While I think I can implement your connection detection strategy, I can imagine periods in my game where the server wouldn't constantly be sending messages, and I'd like to catch a disconnect quickly. I believe the server already sends heartbeat packets to the client to maintain the WebRTC connection, and it feels weird to be manually sending my own heartbeat packets on top of that to detect disconnects.

You don't need to send heartbeat packets, probably if you're using UDP-like protocols for a game you're sending game updates constantly right? Otherwise, UDP is probably not the best protocol anyway. You'll notice a disconnection because a send will fail.

[kyren]

I made a quick branch (afa84ba20977ef9ce464cfe93554baaecf773e67) with some of the changes I was talking about but I'm not sure I like the fact that the events and the behavior of the Server still present two differing models of client state, and the real state transitions of the client are way too much and kind of confusing.

[kyren]

Actually, now that I think it through some more, I'm not even sure I definitely want such an API. I think if you want to think of this library as UDP but for the web then the connection event API is not that useful.

How I've been using this is actually as an alternative to plain UDP, so since plain UDP is completely connectionless, having this API only have an implicit connection is really not a very big deal.

I think having an event for a new, established connection is maybe useful, and the 'events' branch certainly has that and it works, but events on disconnection are kind of messy. They're also not terribly helpful, as it's kind of hard to get a disconnection reason other than a timeout in my testing, but maybe that's a sign of other bugs.

I think the most sensible API I could make is what's in the current 'events' branch, it's just not very useful. I think the only thing it would allow you to do is have a protocol where the server talked first, but I don't actually think that's a good idea anyway, especially when you consider this as UDP for the web.

[connorcarpenter]

Hey Kyren, thanks for thinking through this and getting this working! I totally follow on the idea that as a UDP replacement, perhaps this library should provide a connectionless API. Of course, it appears to me that WebRTC does maintain a connection, so why not have some visibility into that? If UDP for Web is the goal, it does make me wonder if trying to emulate the std::net::UdpSocket API would be a good idea? But since WebRTC does maintain a connection, would that route would lead to inefficiencies? Anyway, I'm completely believe webrtc-unreliable should be THE Rust engine for WebRTC datachannel servers, and towards that cause, I personally believe performance & keeping it opinionated is the way to go. I'm personally at a standstill as to my own opinion of connection & disconnection events in here - just a lack of expertise I guess.

But no worries, I'm going to continue experiment with getting connection/disconnection events working at a higher layer as per your suggestions! Thanks for checking into this, and I think your branch could help someone else looking to implement their own connection events on top of this. Feel free to close this :+1:

[kyren]

Of course, it appears to me that WebRTC does maintain a connection, so why not have some visibility into that?

Definitely, I mean you can query whether a connection is established or not already.

Maybe there's a good middle ground, what if I just provided you an API to ask what established connections there are, and then at the very least you could poll that every second or something? You're not the first person to ask for that actually and that's quick and uncontroversial. Would that be helpful?

Anyway, I'm completely believe webrtc-unreliable should be THE Rust engine for WebRTC datachannel servers, and towards that cause, I personally believe performance & keeping it opinionated is the way to go.

I dunno if I'm trying to be opinionated or unopinionated or whether I'm just having a hard time coming up with a good API.

I do eventually hope there will be a pure rust WebRTC library that implements more of the standard.

But no worries, I'm going to continue experiment with getting connection/disconnection events working at a higher layer as per your suggestions! Thanks for checking into this, and I think your branch could help someone else looking to implement their own connection events on top of this. Feel free to close this 👍

I'm not gonna close it, I'm gonna leave it up until I either merge the 'events' branch or drop it.

Thanks for being so patient with me while I waffle about what API to have 👍

[connorcarpenter]

No worries about API decisions, I'm a chronic waffler on that front too :P I do feel like eventually I will want a list of established connections exposed, so yeah that would be awesome! And yeah, I think I'd be able to wire up connection/disconnection events that way, but I'll probably keep a TODO around to get away from polling for those events, you know, to avoid performance issues once I have hundreds of thousands of active users playing my game & connected to my server haha :P

[kyren]

No worries about API decisions, I'm a chronic waffler on that front too :P I do feel like eventually I will want a list of established connections exposed, so yeah that would be awesome!

It's there now, I pushed it shortly after I said it.

And yeah, I think I'd be able to wire up connection/disconnection events that way, but I'll probably keep a TODO around to get away from polling for those events, you know, to avoid performance issues once I have hundreds of thousands of active users playing my game & connected to my server haha :P

I don't think you can get hundreds of thousands on a single Server instance anyway, hundreds of thousands of UDP clients is a pretty tall order! (if you ever hit limits of a single Server instance though you can round robin them)

[connorcarpenter]

Thank you Kyren, that exposed list of clients will definitely work for my needs for now, I appreciate it 👍

I would be interested in how round-robin-ing would work! I guess you'd need to off-load the list of connected clients somewhere to have a singular source of truth? I'm sure there's more that needs to be synced between the Server instances? I just know it may be years before I ever require something like that, but it's pretty cool that'd be an option :)

[kyren]

I would be interested in how round-robin-ing would work! I guess you'd need to off-load the list of connected clients somewhere to have a singular source of truth? I'm sure there's more that needs to be synced between the Server instances? I just know it may be years before I ever require something like that, but it's pretty cool that'd be an option :)

Uhh, it depends on how you wire up webrtc-unreliable to the rest of your game, but if a connected client eventually becomes some kind of packet channel (like a futures mpsc channel), then round-robin-ing between Server instances is pretty easy, you'd just have say 8 Server instances on 8 ports, so the HTTP connection API can give out one of those 8 ports, and then each client will then connect to one of the 8 Server instances. At some point something is going to have to deliver new connections somehow (maybe over a channel again?), and those should themselves contain a channel of packets being driven by some other task, so that way you can listen for new connections that ultimately come from more than one Server at a time and share the load. That's one way of doing it but there are probably a lot of ways to do it.