Puffin_http: Add wait client feature
Draft as it's should be better to finish and merge https://github.com/EmbarkStudios/puffin/pull/256 first. And the preparation changes need to be cleaned.
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
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
Hi! I am interested in this feature so I decided to review your changes.
Here is what I have found:
tcp_listeneris 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
Servercreation. - 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
Clientand the corresponding thread seems to be effectively leaked because the connection accepting thread ownsPuffinServerConnectionand 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.
I tried to implement this on my own: https://github.com/EmbarkStudios/puffin/pull/264