An improvement in send_ctrl_c_event so it's less likely to drop the ctrl event#307
An improvement in send_ctrl_c_event so it's less likely to drop the ctrl event#307catoxliu wants to merge 2 commits into
Conversation
Wait before FreeConsole to make it less likely to drop the Ctrl event.
GenerateConsoleCtrlEvent dispatches the CTRL_C_EVENT asynchronously
via a system thread pool. The original 5ms sleep before FreeConsole()
was often too short for the async dispatch to complete, causing the
signal to be lost before reaching the target console's process group.
Moved it before FreeConsole() so we
remain attached to the child's console while the async thread finishes.
psmux itself is protected during this window by the preceding
SetConsoleCtrlHandler(None, 1) which ignores the signal.
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Windows send_ctrl_c_event implementation to reduce the chance of losing a CTRL_C_EVENT by sleeping briefly before FreeConsole(), keeping the process attached to the child’s console while GenerateConsoleCtrlEvent’s asynchronous dispatch has time to complete.
Changes:
- Move the existing 5ms sleep to occur before
FreeConsole()afterGenerateConsoleCtrlEvent. - Expand inline commentary explaining why the sleep is necessary to reduce event loss risk.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let err = GetLastError(); | ||
|
|
||
| log(&format!("GenerateConsoleCtrlEvent => ok={} err={}", ok, err)); | ||
|
|
| // it returns as soon as the signal is queued for delivery, not when it's actually handled by the target process. | ||
| // FreeConsole will reset the table of control handlers in the client process. | ||
| // If current process finishes detaching before the console subsystem finishes distributing the event, | ||
| // the context tracking who generated the signal is destroyed, |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Move the 5ms sleep from after FreeConsole() to before it, so psmux remains attached to the child's console while the async Ctrl+C dispatch completes. GenerateConsoleCtrlEvent returns as soon as the signal is queued, not when it is handled. Detaching too early could cause the console subsystem to lose the signal context. psmux is protected during the sleep by the preceding SetConsoleCtrlHandler(None, 1) which ignores Ctrl+C. Adds E2E test with 50 iterations of Ctrl+C signal delivery verification. Co-authored-by: Shijian Liu <catoxliu@gmail.com>
|
Hey @catoxliu, thanks for this! Great catch on the ordering. You're absolutely right that sleeping AFTER I ran a 50 iteration stress test (rapid fire Applied and pushed in 9346519 with your Co-authored-by credit. Thanks again for the contribution, Shijian! |
1 similar comment
|
Hey @catoxliu, thanks for this! Great catch on the ordering. You're absolutely right that sleeping AFTER I ran a 50 iteration stress test (rapid fire Applied and pushed in 9346519 with your Co-authored-by credit. Thanks again for the contribution, Shijian! |
Wait before FreeConsole to make it less likely to drop the Ctrl event.
Moved it before FreeConsole() so we
remain attached to the child's console while the async thread finishes.
psmux itself is protected during this window by the preceding
SetConsoleCtrlHandler(None, 1) which ignores the signal.