Skip to content

Conversation

@im-0
Copy link
Contributor

@im-0 im-0 commented Sep 16, 2025

Checklist

  • I have read the Contributor Guide
  • I have read and agree to the Code of Conduct
  • 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

Alternative pull request for the same feature: #257

@im-0 im-0 mentioned this pull request Sep 16, 2025
3 tasks
@im-0 im-0 force-pushed the server-refactoring branch from 5bb1583 to c455167 Compare September 16, 2025 21:54
@im-0
Copy link
Contributor Author

im-0 commented Sep 16, 2025

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
Copy link
Contributor Author

im-0 commented Sep 16, 2025

OH NO IT HANGS ON MACOS!..

This is a backward-compatible change.

Changes:

* Added an ability to call a user-supplied callback when first client
  connects or last client disconnects.
* Added tests for the new callback mechanism.
* Socket listener decoupled from the profiling data
  processing/forwarding.
* Dependency on `crossbeam_channel` removed.
* Added function to get an actual IP:Port that server is using.
* Sockets are properly shut down to prevent data loss.
* Panics inside the service threads are propagated further now.
* Added panics when some impossible states are encountered.

This fixes EmbarkStudios#85
and maybe EmbarkStudios#172
Otherwise parallel doc tests may fail with "Address already in use".
@im-0 im-0 force-pushed the server-refactoring branch from c455167 to d267a4e Compare September 16, 2025 23:18
@im-0
Copy link
Contributor Author

im-0 commented Sep 16, 2025

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.

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.

1 participant