Fix shutdown deadlock in PicoQuicTransport and qclient signal handling#767
Fix shutdown deadlock in PicoQuicTransport and qclient signal handling#767bifurcation wants to merge 3 commits intoQuicr:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3162f3b0d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| while (not stop_threads) { | ||
| while (not stop_threads && not moq_example::terminate) { | ||
| if (client->GetStatus() == MyClient::Status::kReady) { |
There was a problem hiding this comment.
In the connect loop, moq_example::terminate is now read without holding main_mutex, but the signal handler writes it under that mutex (signal_handler.h). If a SIGINT arrives while this loop is running, the handler can write concurrently with this unsynchronized read, which is a data race/UB and can cause missed termination or undefined behavior. Consider making terminate an std::atomic<bool> or taking the same mutex when reading it here.
Useful? React with 👍 / 👎.
|
|
||
| // Signal thread to exit BEFORE waiting | ||
| quic_network_thread_ctx_->thread_should_close = 1; | ||
| picoquic_wake_up_network_thread(quic_network_thread_ctx_); | ||
|
|
||
| while (quic_network_thread_ctx_->thread_is_ready) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(1)); | ||
| } |
There was a problem hiding this comment.
I'm not sure any of this is correct. Calling picoquic_delete_network_thread will set thread_should_close to 1 on its own. I'd say we should let library handle it's graceful closing (i.e. remove all this weirdness and just call the delete).
3 participants
Currently, PicoQuicTransport::Shutdown() contains a deadlock that prevents graceful process termination. The code waits for the picoquic network thread to exit (
thread_is_ready == 0) before callingpicoquic_delete_network_thread(), but the thread only exits whenthread_should_closeis set — which only happens insidepicoquic_delete_network_thread(). Additionally, the qclient example holds a mutex during its connection loop that blocks the signal handler from executing, preventing Ctrl-C from working.This PR fixes both issues: it sets thread_should_close = 1 before the busy-wait loop in Shutdown(), and moves the mutex acquisition in qclient to after the connection loop while also checking the terminate flag during connection attempts. Together, these changes allow qclient (and any application using libquicr) to shut down cleanly when interrupted with Ctrl-C.