Refactor puffin_http::Server to support client connect/disconnect callback
Checklist
- [x] I have read the Contributor Guide
- [x] I have read and agree to the Code of Conduct
- [x] I have added a description of my changes and why I'd like them included in the section below
Description of Changes
I need the ability to enable profiling when puffin_viewer connects and disable when it disconnects so I decided to add a callback to support this use case (and possibly more). Implementation turned out to be more complex than I expected and I rewrote the server almost entirely.
However, this is a backward-compatible change. No major version increment required.
Summary of changes:
-
Added
Server::set_on_state_change(),Server::unset_on_state_change()andServer::replace_on_state_change()for setting a callback that is called when first client connects or last client disconnects from server. See documentation ofServer::set_on_state_change()for the details. -
Added test for this new callback mechanism.
-
Socket listener decoupled from the profiling data processing/packet forwarding and separated into its own thread.
-
Dependency on
crossbeam_channelremoved.Original code contained this comment:
// We use crossbeam_channel instead of `mpsc`, // because on shutdown we want all frames to be sent. // `mpsc::Receiver` stops receiving as soon as the `Sender` is dropped, // but `crossbeam_channel` will continue until the channel is empty.This is false. Std's
mpscchannels do not work like this. This is easy to test: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=2ae8d03c8910d6b709a5ef7780a433be -
Added
Server::local_addr()to get an actual IP:Port that server is using. Useful when server is started on a random available port ("localhost:0"). -
TcpStream::shutdown()is used to ensure no data loss when closing client sockets. This is very implementation defined, such problem may never happen in practice. -
Panics inside the service threads are propagated further now.
-
Added panics when some impossible states are encountered. Those can fire only if I introduced bugs somewhere.
-
Server example updated to show the usage of new
Serverfunctions. -
$CLIENT is not accepting data fast enough; dropping a framewarning will be shown only once per client to not flood the logs. -
Added 30-second TCP write timeout. Timed out clients are dropped.
-
I tried to minimize the amount of reallocations in the packet fan-out thread by keeping the maximum packet size for
Vec::with_capacity().
There is a single problem with new implementation though: it is impossible to reliably shut down a TCP listener without adding additional dependencies for a proper async networking. I tried my best, but it may break in some cases:
- New code tries to wake up the listener thread by creating a new
TcpStreamto aServer::local_addr()with some small timeout. It may fail with a timeout if everything runs horribly slow for some reason. - The "ping to wake up" mechanism may also fail is server was bound to some IP address that was later removed from the system's networking interface.
In those cases, handle of the listener thread is added into the LEAKED_LISTENERS table. This table is checked when socket bind fails with "Address already in use" while creating new Server.
Related Issues
- https://github.com/EmbarkStudios/puffin/issues/85
- and maybe https://github.com/EmbarkStudios/puffin/issues/172
Alternative pull request for the same feature: https://github.com/EmbarkStudios/puffin/pull/257
Fixed
error[E0658]: `let` expressions in this position are unstable
--> puffin_http/src/server.rs:579:27
|
579 | if had_clients && let Some(mut on_state_change) = on_state_change {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #53667 <https://github.com/rust-lang/rust/issues/53667> for more information
Let-chains are only available since rustc 1.88.0.
OH NO IT HANGS ON MACOS!..
macOS fixed!
It turns out you have to keep the TcpStream from TCP ping around while joining the listener thread. accept() may never return if you close the "successfully connected" client socket too early.