puffin icon indicating copy to clipboard operation
puffin copied to clipboard

Refactor puffin_http::Server to support client connect/disconnect callback

Open im-0 opened this issue 3 months ago • 3 comments

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() and Server::replace_on_state_change() for setting a callback that is called when first client connects or last client disconnects from server. See documentation of Server::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_channel removed.

    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 mpsc channels 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 Server functions.

  • $CLIENT is not accepting data fast enough; dropping a frame warning 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 TcpStream to a Server::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

im-0 avatar Sep 16 '25 21:09 im-0

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.

im-0 avatar Sep 16 '25 21:09 im-0

OH NO IT HANGS ON MACOS!..

im-0 avatar Sep 16 '25 22:09 im-0

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.

im-0 avatar Sep 16 '25 23:09 im-0