servers: drop duplicate (and interacting) ctrl handlers on Windows, add exit message - #22487
servers: drop duplicate (and interacting) ctrl handlers on Windows, add exit message#22487vszakats wants to merge 27 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies Windows test-server termination handling by removing duplicate/overlapping signal-based control-handler registration, relying on a single SetConsoleCtrlHandler() path and consolidating “exit event” signaling behavior.
Changes:
- Removes Windows
signal()registrations that indirectly add extra console CTRL handlers. - Moves/centralizes Windows exit-event signaling logic into
ctrl_event_handler(). - Refactors
install_signal_handlers()/restore_signal_handlers()to split Windows vs non-Windows behavior more cleanly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Windows code no longer installs any SIGTERM/SIGINT handlers, but the hidden-window WM_CLOSE path still calls raise(signum) (util.c:459-462). Without a handler, raise(SIGTERM) will use the default action and can terminate the process immediately, skipping the normal got_exit_signal/exit_event-driven shutdown and cleanup that the test servers rely on." curl#22487 (comment)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/server/util.c:661
exit_eventis created on Windows regardless ofCURL_WINDOWS_UWP, but it is only closed inside the#ifndef CURL_WINDOWS_UWPblock. WhenCURL_WINDOWS_UWPis defined this leaks the handle and leavesexit_eventnon-NULL for the remainder of the process lifetime. Closeexit_eventoutside theCURL_WINDOWS_UWPguard (the guard should only apply to the hidden-window thread logic).
if(exit_event && CloseHandle(exit_event))
exit_event = NULL;
#endif
|
The usual news, yes, this also does not fix "broken fork" CI errors (hangs): https://github.com/curl/curl/actions/runs/30996302837/job/92274126158?pr=22487 |
Also: shorten code. Reported by Copilot Bug: #22487 (review) Closes #22489
Windows code no longer installs any SIGTERM/SIGINT handlers, but the hidden-window WM_CLOSE path still calls raise(signum) (util.c:459-462). Without a handler, raise(SIGTERM) will use the default action and can terminate the process immediately, skipping the normal got_exit_signal/exit_event-driven shutdown and cleanup that the test servers rely on." curl#22487 (comment)
ada5abf to
d44d763
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/server/util.c:663
- In restore_signal_handlers() on Windows, exit_event is closed twice in the non-UWP build (once inside the !CURL_WINDOWS_UWP block and again immediately after). This is redundant and makes the control flow harder to follow (and will retry CloseHandle() only on failure of the first call). Close the handle in a single place outside the !CURL_WINDOWS_UWP block.
if(exit_event && CloseHandle(exit_event))
exit_event = NULL;
#endif
if(exit_event && CloseHandle(exit_event))
exit_event = NULL;
Windows code no longer installs any SIGTERM/SIGINT handlers, but the hidden-window WM_CLOSE path still calls raise(signum) (util.c:459-462). Without a handler, raise(SIGTERM) will use the default action and can terminate the process immediately, skipping the normal got_exit_signal/exit_event-driven shutdown and cleanup that the test servers rely on." curl#22487 (comment)
d44d763 to
1de8739
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/server/util.c:396
- exit_signal_handler() is documented as only calling async-signal-safe functions, but this error path calls strlen(serverlogfile), which is not async-signal-safe per POSIX and can lead to undefined behavior when invoked from a signal handler. Avoid strlen here by writing the path with a simple byte loop.
static const char msg[] = "exit_signal_handler: failed opening ";
(void)write(STDERR_FILENO, msg, CURL_CSTRLEN(msg));
(void)write(STDERR_FILENO, serverlogfile, strlen(serverlogfile));
(void)write(STDERR_FILENO, "\n", 1);
`strlen()` is only guaranteed to be signal-safe since POSIX.1-2008. Ref: https://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html#tag_02_04_03 Reported by Copilot Bug: #22487 (review) Follow-up to e95f509 #16852 Closes #22491
Windows code no longer installs any SIGTERM/SIGINT handlers, but the hidden-window WM_CLOSE path still calls raise(signum) (util.c:459-462). Without a handler, raise(SIGTERM) will use the default action and can terminate the process immediately, skipping the normal got_exit_signal/exit_event-driven shutdown and cleanup that the test servers rely on." curl#22487 (comment)
Windows code no longer calls `signal()`, therefore the `raise()` call is now a no-op.
This reverts commit 084c48b.
c9ea16c to
0633bea
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/server/first.c:62
exit_msgstrings being assigned include a trailing\n(e.g. in the Windows ctrl/window handlers andexit_signal_handler()), butfirst.clater logs them vialogmsg("... %s", exit_msg), which will embed a newline into the formatted log line and break the log format (double/newline-split lines). Consider stripping a trailing newline before logging (or store messages without newlines).
if(serverlogfile && exit_msg)
logmsg("========> exit message: %s", exit_msg);
tests/server/util.c:488
main_window_proc()callsinitiate_exit(SIGTERM)but ignores its return value. Ifexit_eventfailed to initialize (rare but explicitly handled inctrl_event_handler()),initiate_exit()returns FALSE and the server may keep blocking inWaitForMultipleObjectsEx()/select_ws()after WM_CLOSE. Consider adding a fallback to an ungraceful shutdown here too (consistent with the ctrl handler’s “avoid hang” behavior).
&dwWritten, NULL);
exit_msg = msg;
initiate_exit(SIGTERM);
break;
I'm aware. I don't think this deserve spending more effort on. There will be an extra newline there.
I'm also aware, but no idea what to do here as a fallback. If you do, please shout! |
|
I get now a terminal full scroll of these exit messages since your previous change at the end of test runs before the summary result. |
Thanks, I think I got it, runtests always signals the server, so after each server shutdown there is one. |
|
I'll add it to this PR, to avoid reversing order of two conflicting patches. This one adds an exit_msg, and makes the stderr output redundant anyway. |
|
...and dang, this also did not fix the 2304 flaky fail: https://github.com/curl/curl/actions/runs/31097520068/job/92602997991?pr=22487 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
tests/server/util.c:443
exit_msgis overwritten on every CTRL event (including during shutdown), whileexit_signalis only captured on the first event. This can lead to confusing/mismatched exit logs. Consider only settingexit_msgonce (first event wins).
WriteFile(out, msgU, CURL_CSTRLEN(msgU), &dwWritten, NULL);
exit_msg = msgU;
return FALSE;
}
if(!initiate_exit(signum)) {
tests/server/util.c:475
- In the WM_CLOSE path, the message stored in
exit_msgincludes a trailing newline (which will create embedded newlines when later logged vialogmsg()), and it can also overwrite a previously recorded exit message. Store the message without\nand set it only if it hasn't already been set.
static const char msg[] = "main_window_proc(): WM_CLOSE -> SIGTERM\n";
DWORD dwWritten;
WriteFile(GetStdHandle(STD_ERROR_HANDLE), msg, CURL_CSTRLEN(msg),
&dwWritten, NULL);
exit_msg = msg;
tests/server/util.c:424
- The strings assigned to
exit_msgin the Windows handlers include a trailing\n. Sincefirst.clater logsexit_msgvialogmsg()(which appends its own newline), this will produce embedded newlines and break log line formatting. Prefer storing messages without trailing newlines (you can still include newlines in the directWriteFile()output if desired).
static const char msgU[] = "ctrl_event_handler(): unhandled\n";
static const char msgH[] = "ctrl_event_handler(): handled\n";
static const char msgF[] = "ctrl_event_handler(): failed to handle\n";
tests/server/util.c:374
exit_signal_handler()setsexit_msgunconditionally even thoughexit_signalis only captured once. To keep the exit message consistent with the first terminating signal (and avoid later signals overwriting it), only setexit_msgwhen it hasn't already been set.
This issue also appears in the following locations of the same file:
- line 439
- line 471
exit_msg = "exit_signal_handler(): triggered";
(void)initiate_exit(signum);
tests/server/util.c:344
- The MSVC <= 1700 workaround for
SIG_ERRwas removed. Older MSVC definesSIG_ERRin a way that can trigger warning C4306 when assigning toSIGHANDLER_T, which can break builds that treat warnings as errors. Consider restoring the guarded redefinition for those compilers.
typedef void (*SIGHANDLER_T)(int);
To simplify and to avoid the chance of potential interference or thread-safety issues. If one these 3 Win32 API calls fail, there is likely a serious problem, out of the code's control. Knowing `GetLastError()` is unlikely to help. Refs: https://learn.microsoft.com/windows/win32/api/winuser/nc-winuser-wndproc https://learn.microsoft.com/previous-versions/windows/desktop/legacy/ms686736(v=vs.85) https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getmessage https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-createwindowexa https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-registerclassa Ref: 9ea4881 #22487 Ref: 1c49f2f #18451 Follow-up to ac1e206 Closes #22045
|
Noting that after this patch, in one particular Windows CI job that Ref: https://github.com/curl/curl/actions/runs/31132202463/job/92723553053#step:14:4002 |
On Windows, the init code calls
SetConsoleCtrlHandler(), and beforethis patch also set handlers for all Unixy signals. Of these,
SIGBREAK(used on Windows-only),
SIGINT,SIGABRTandSIGTERMwere alsosetting up a
SetConsoleCtrlHandler(), in addition to the call madedirectly. (The rest,
SIGHUP,SIGPIPE,SIGALRMare either missingthe macros, or ignored by
signal()on Windows.)As per WINE sources,
SetConsolCtrlHandler(<h>, TRUE)calls areadditive, which means the test server set up two console ctrl handlers.
Then the ctrl handler set directly (
ctrl_event_handler()), wastriggering the other signal handler via
raise(), for the 'initiateexit' logic, which in turn triggered exiting a wait within
select_ws()and other loops. The Windows window handler also made use of the
SIGTERMevent to initiate exit viaraise()and the second signalhandler.
To simplify, de-duplicate the ctrl handlers by dropping
signal()callsand keeping the direct Win32 call with
ctrl_event_handler()doing allthe signal handling on Windows. Break out the 'initiate exit' logic into
a function and call it from both Unix and Windows signal/ctrl/window
handlers. Also drop calling
raise()on exit, because it's a no-opwithout a
signal()pair.Also:
whether we handled the event, in
ctrl_event_handler(). To avoidusing non-signal-safe functions (e.g.
fprintf()) from the handler.logmsg()withWriteFile()to prevent regressions.Ref: servers: drop CRT and curlx calls from
main_window_loop()(Windows) #22045logmsg()withWriteFile()inmain_window_proc().exit_eventinitialization on startup. To swap a possible hang(within
WaitForMultipleObjectsEx()) with an ungraceful shutdown.and log it on app exit. To avoid the need to deal with logging within
the handlers, yet have a static trace message about the event.
Complementing the already logged signal number.
exit_signal_handler()in favor of anexit message. runtests triggers it frequantly, which added much noise
to stderr. As a bonus, this also allows dropping the compiler warning
suppression.
Reported-by: Stefan Eissing
Bug: servers: drop duplicate (and interacting) ctrl handlers on Windows, add exit message #22487 (comment)
Follow-up to 3aae64e servers: drop complex and redundant signal handler output #22507
Refs:
https://learn.microsoft.com/windows/console/setconsolectrlhandler
https://learn.microsoft.com/windows/console/registering-a-control-handler-function
https://learn.microsoft.com/cpp/c-runtime-library/reference/raise
https://learn.microsoft.com/cpp/c-runtime-library/reference/signal
https://gitlab.winehq.org/wine/wine/-/blob/wine-11.14/dlls/kernelbase/console.c#L1517-1526
https://github.com/huangqinjin/ucrt/blob/d6e817a4cc90f6f1fe54f8a0aa4af4fff0bb647d/misc/signal.cpp#L286-L348
Follow-up to fe28fcf 7dc8a98 0e05877 #5260
WM_CLOSEcodepath works.It should work now like it did before, but without going through a raise/catch custom SIGTERM
cycle, and instead doing the same job directly. I suppose this should be enough without doing any
explicit things, such as generating a CTRL_BREAK_EVENT with
GenerateConsoleCtrlEvent(),or some other things from the window handler?
→ Works fine in local tests, above tricks not needed. The closing
raise(SIGTERM)is rundant, no-op.signal()use limited to non-Windows, itbecomes an option to drop legacy
signal()support in favor ofsigaction()+SA_RESTART,to simplify the source, and drop detection from the build systems.
Not so fast, libcurl code also uses
signal(), making the benefits minimal. [SKIP]OR, if we don't bother, the window handler code can be dropped.
The graceful shutdown is indeed better because server.exe cleans up after
itself, deletes PID file and so on. → [contemplating options at servers: add option to build without window handler (Windows) #22496]