bpo-23057: Use 'raise' to emulate ctrl-c in proactor tests by vladima · Pull Request #11274 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
FYI I spent an enormous amount of time trying to get synthetic control-C to work reliably for tests in Trio, with CTRL_C_EVENT and CREATE_NEW_PROCESS_GROUP and GenerateConsoleCtrlEvent and all kinds of things, and never could get any of it to work reliably. There are some notes on my attempts and failures here and here.
What I eventually realized is: normally, when a CTRL_*_EVENT arrives on Windows, the kernel spawns a new thread in the target process, and that thread invokes whatever handlers were registered with SetConsoleCtrlHandler. One of those handlers is set up by default by the C runtime, which intercepts CTRL_C_EVENT and calls raise(SIGINT), which then invokes whatever handler is registered for the SIGINT signal, which is where we enter the Python interpreter and signalmodule.c and all that stuff.
So if you want to simulate this for testing, you can do that by starting a new thread and using it to call raise(SIGINT) directly. It's almost exactly the same, and in particular triggers exactly the same code paths inside the interpreter as a regular signal does, but avoids touching the maddening Windows console layer.
There is one caveat, which is that if you have code that uses SetConsoleCtrlHandler to directly install its own control-C handlers without going through the C runtime, then this won't run them. Back in the Python 2 days the interpreter used to do this in some cases. But in modern Python these have all been removed.
Use CREATE_NEW_PROCESS_GROUP for proactor tests bpo-23057: Use CREATE_NEW_PROCESS_GROUP for proactor tests
@njsmith what do you suggest?
Merge the PR or reimplement the test to follow your recommendations?
I'm not sure :-(. The frustrating thing is that I never did figure out why I was getting such weird effects or how to avoid them. So... it's possible this patch is somehow using exactly the right incantation and will work perfectly for everyone forever? It's promising that both Appveyor and Azure are green – when I tried I could get it working reliably locally but never on Appveyor, so this is already doing better than that.
So it's a judgement call... merging as is has some risk, or it might be fine. Rewriting should definitely be fine, but obviously requires more work.
Interesting, I've initially considered using raise(SIGINT) but the bit that stopped me was that I need to load debug/release version of the CRT depending on build flavor.
import ctypes
import signal
# load debug version of CRT
# comment this line and uncomment one below to load release CRT
crt = getattr(ctypes.cdll, "ucrtbased.dll")
#crt = getattr(ctypes.cdll, "ucrtbase.dll")
f_raise = getattr(crt, "raise")
f_raise.argtypes = [ctypes.c_int]
f_raise.restype = ctypes.c_int
try:
print("before")
f_raise(signal.SIGINT)
print("after - not ok")
except KeyboardInterrupt:
print("after - ok")
Script above works correctly with debug version of Python and fails with release. To make it work with a release flavor (and break debug) - commend loading of debug CRT and uncomment a line that loads release C runtime. NOTE: per this discussion loading named exports from ucrtbase.dll might fail in the future (works for now) however looks like using api-ms-win-crt-runtime-l1-1-0.dll always loads raise from ucrtbase.dll and thus always fails with debug builds.
I've tried CREATE_NEW_PROCESS_GROUP based version on Win10/Win7 and it works pretty reliably however if people feel very strongly that using raise(SIGINT) is a right path - I can switch to using it (assuming that there is a consensus for a question of picking proper CRT). One option that should make it more robust is if raise will be exposed in msvcrt module but I was not sure what is the bar for adding new functions to the public API.
What I eventually realized is: normally, when a
CTRL_*_EVENTarrives on Windows, the kernel spawns a new thread in the target process,
Console control events are implemented mainly in user mode from start to finish, but the kernel is of course involved with interprocess messaging and thread creation. In particular, the console host (conhost.exe) sends an LPC message to the session server (csrss.exe) to request the creation of a control thread. This thread executes the undocumented CtrlRoutine function. CtrlRoutine calls the registered handler functions, or simply does nothing if it's a Ctrl+C event and the ignore flag is enabled. If no handler returns true to indicate an event was handled, the default handler exits the process with the code STATUS_CONTROL_C_EXIT.
WinAPI GenerateConsoleCtrlEvent is roughly equivalent to POSIX killpg. POSIX leaves group 0 as undefined. In Windows, group 0 includes all processes currently attached to the console. To simulate a user pressing Ctrl+C, call GenerateConsoleCtrlEvent(CTRL_C_EVENT, 0) or os.kill(0, signal.CTRL_C_EVENT).
that thread invokes whatever handlers were registered with
SetConsoleCtrlHandler. One of those handlers is set up by default by the C runtime, which interceptsCTRL_C_EVENTand callsraise(SIGINT),
CtrlRoutine is actually exported by kernel32.dll. With the caveat that calling an undocumented function is rarely a good idea, it's worth mentioning that we can call this function on a new thread to faithfully simulate the arrival of a control event. For example:
import signal
import threading
import ctypes
kernel32 = ctypes.WinDLL('kernel32', use_last_error=True)
t = threading.Thread(target=kernel32.CtrlRoutine, args=(signal.CTRL_C_EVENT,))
t.start()
t.join()
Oh, that's interesting... right now Trio unconditionally uses api-ms-win-crt-runtime-l1-1-0.dll, so I guess it's broken on Windows debug builds and it's just that no-one noticed. (And unfortunately, this breakage isn't restricted to the test suite – Trio calls raise during normal operation too.) Is it even possible for a running Python script to figure out which version of the CRT the interpreter is linked against?
I was not sure what is the bar for adding new functions to the public API.
@zooba at least argues for using a low bar to add things to msvcrt in this message. Though... there's nothing Windows-specific about raise – it's actually required by C89! The signal module would probably be a better place for it.
Filed a bug for that: https://bugs.python.org/issue35568
Oh, that's interesting... right now Trio unconditionally uses
api-ms-win-crt-runtime-l1-1-0.dll, so I guess it's broken on Windows debug builds and `it's just that no-one noticed
There's no capability in the loaders's API-set schema to dynamically map this name to "ucrtbased.dll". In a release build, prefer the API set. In a debug build, use "ucrtbased.dll". (Notice that the PE import tables for debug builds link with ucrtbased.dll instead of to the CRT API sets.) For cross-platform code, use hasattr(sys, 'gettotalrefcount') to check for a debug build. Specifically for Windows, a debug build has "_d.pyd" in importlib.machinery.EXTENSION_SUFFIXES.
@eryksun I suspect you're right that that's the best option currently available, but is there really a hard rule that Py_REF_DEBUG is set if-and-only-if we are linking against ucrtbased.dll?
looks like raise is already exposed for tests so I can convert this PR to use it.
Also tests usually check Py_DEBUG = hasattr(sys, 'gettotalrefcount') or Py_DEBUG = hasattr(sys, 'getobjects')
bpo-23057: Use CREATE_NEW_PROCESS_GROUP for proactor tests bpo-23057: Use 'raise' to emulate ctrl-c in proactor tests
…ng unregistration that was also fixed.
is there anything else that should be done in this PR?
@njsmith, @asvetlov - can you please tell if there is anything else that should be added/changed in this PR?
Another way to raise KeyboardInterrupt in the main thread is _thread.interrupt_main(). It's the most high-level way since it trips the interpreter's signal flag directly, rather than going through C raise or GenerateConsoleCtrlEvent.
Note that the original code that this PR replaces is relying on buggy behavior. It uses os.kill to call GenerateConsoleCtrlEvent to send CTRL_C_EVENT to a process ID that's not a console-process group ID. (The original version of this PR fixed this problem directly by creating a new group.) This used to reliably fail in older versions of Windows NT, prior to XP. Now it kind of succeeds, but only if the process happens to be a child of another process that's attached to the console. If the target is also attached to the console, it behaves like sending to console-process group 0. If it's not attached to the console, there's no immediate effect; however, the target process gets added to the console's process list (i.e. GetConsoleProcessList), which breaks future GenerateConsoleCtrlEvent calls once the target process has terminated (thus leaving the console with a handle for a finalized process). Due to this bug, GenerateConsoleCtrlEvent should only be called for either console-process group 0 or a process that was created with the flag CREATE_NEW_PROCESS_GROUP and is attached to the current console.
Another way to raise KeyboardInterrupt in the main thread is _thread.interrupt_main(). It's the most high-level way since it trips the interpreter's signal flag directly, rather than going through C raise or GenerateConsoleCtrlEvent.
IIRC when I looked at this I decided that interrupt_main was too dissimilar to a real signal to be a useful test. Most obviously on Unix it doesn't trigger EINTR, but also maybe it doesn't write to the wakeup fd or something? I think there's a bpo issue somewhere discussing reimplementing interrupt_main using pthread_kill on posix and raise on Windows.
This PR desperately needs to be addressed! The 2 Windows 7 buildbots have been failing on 3.x since the merge of #11135 on 12-18. Either that or the commit b5c8cfa needs to be reverted.
I'm willing to merge this, but I'm not clear why it's been held up? Is anyone else in this discussion planning to merge or are you just chatting now? If it's just chat, better to go on the bug rather than here, since this will all be lost once it's merged.
is there anything else that should be done for this PR?
I think everything is fine.
Thank you, @vladima
@asvetlov: Please replace # with GH- in the commit message next time. Thanks!