Skip to content

Conversation

@gwen-lg
Copy link
Contributor

@gwen-lg gwen-lg commented Aug 25, 2025

Draft as it's should be better to finish and merge #256 first.
And the preparation changes need to be cleaned.

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

Add a wait_client() function on puffin_http::Server to be able to pause profiled application while the client (ex: puffin_viewer) is connected.
This allow to avoid to lost first frames.
Move accept_new_clients() out of the frame reception loop and in is own thread, otherwise it won't work.

Related Issues

List related issues here

@gwen-lg gwen-lg force-pushed the puffin_http_wait_client branch from d0f0642 to 9938819 Compare August 27, 2025 11:24
@im-0
Copy link
Contributor

im-0 commented Sep 14, 2025

Hi! I am interested in this feature so I decided to review your changes.

Here is what I have found:

  • tcp_listener is in a non-blocking mode and the the connection accepting thread does not block on anything. This means that this thread will consume 100% of one CPU core constantly.
  • The connection accepting thread is running in a detached mode, never joined, and does not have any ability to be stopped. And it owns the listener socket, which is never destroyed once created. This makes it impossible to start/stop the Puffin server on demand because port will be already used after the initial Server creation.
  • New notification mechanism uses a bounded queue with zero capacity. This means that the connection accepting thread will be stuck on send if no one is waiting. This breaks support of multiple concurrent connections, and support of simply reconnecting a viewer.
  • Every Client and the corresponding thread seems to be effectively leaked because the connection accepting thread owns PuffinServerConnection and it is never destroyed.

Also, I think that there are some usability issues with the new API:

  • There is no way to wake up the thread which is waiting on Server::wait_client() unless someone actually connects the viewer. This makes graceful shutdown impossible in some cases, for example when some other thread encountered a fatal error and wants to terminate the whole app.
  • There is no easy way to wait for a connected viewer in multiple threads simultaneously. It is possible to implement this by creating yet another thread though.

I would prefer this functionality in a form of two callback functions: on_connect and on_disconnect. This will make it possible to use custom synchronization primitives, like conditional variables. And also will allow, for example, enabling/disabling the profiling dynamically based on whether any viewers are connected or not.

@im-0
Copy link
Contributor

im-0 commented Sep 16, 2025

I tried to implement this on my own: #264

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.

2 participants