rust: Add support for ws-protocol time capability #188
Conversation
gasmith
force-pushed
the
gasmith/fg-9671-ws-time
branch
from
4978731 to
36c4005
Compare
| @@ -431,6 +433,28 @@ impl Server { | |||
| } | |||
| } | |||
|
|
|||
| /// Publish the current timestamp to all clients. | |||
| #[cfg(feature = "unstable")] | |||
| pub async fn broadcast_time(&self, timestamp_nanos: u64) { | |||
There was a problem hiding this comment.
We can shelve naming discussions until this moves to the stable API, but we don't necessarily need to follow the server interfaces from ws-protocol here. I like 'broadcast' but also think we should consider this alongside 'publish' parameter values (which may or may not broadcast).
There was a problem hiding this comment.
Yeah. I don't feel strongly about it. Maybe I'll develop stronger opinions as we fill in more of the interface. Consistency is nice.
|
|
||
| let clients = self.clients.get(); | ||
| for client in clients.iter() { | ||
| if let Err(err) = client.control_plane_tx.send_async(message.clone()).await { |
There was a problem hiding this comment.
control_plane_tx seems reasonable for a start, but this does make me wonder if these should be prioritized, and/or dealt with in some way such that we don't queue up multiple time messages.
There was a problem hiding this comment.
Maybe debounce these? That seems to be coming up a lot lately as a future improvement
There was a problem hiding this comment.
Debouncing sounds reasonable, and prioritizing time messages ahead of other message types also seems reasonable, since queueing delays will be manifested as clock skew on the client side. I'm tempted to defer this until we have a better sense of the requirements for other message types.
There was a problem hiding this comment.
I guess this is just how the API works, but I'm surprised the end user had to call broadcast_time explicitly.
I would have expected the server itself maybe publishes the current time periodically.
I'm curious why it works this way.
What do you think of Bryan's recommendation for a name for the method?
The implementation looks good to me. I agree with the test reorganization.
I thought about that too. The ws-protocol spec mention that this can be used to obtain "accelerated, slowed, or stepped control over the progress of time." Which sound a bit funkier than plain old clock synchronization. We could provide a batteries-included helper for periodically broadcasting the wallclock time, if that would be useful. But I guess I'd like to learn more about how this interface is typically used before we do that. |
Add the capability and publishing for server
time.I also moved the websocket tests under the websocket module. Apart from feeling more natural, this also gives us the ability to access
crate::websocket::protocol::serverwithout making it more visible throughout the crate.