Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-23057: Use 'raise' to emulate ctrl-c in proactor tests #11274

Merged
merged 6 commits into from Jan 5, 2019

Conversation

Projects
None yet
8 participants
@vladima
Copy link
Contributor

vladima commented Dec 21, 2018

Switch from using CREATE_NEW_CONSOLE to CREATE_NEW_PROCESS_GROUP in tests for ProactorEventLoop that use ctrl-c. Addresses issue reported in #11135.

https://bugs.python.org/issue23057

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Dec 22, 2018

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.

@njsmith njsmith added the skip news label Dec 22, 2018

@njsmith njsmith changed the title Use CREATE_NEW_PROCESS_GROUP for proactor tests bpo-23057: Use CREATE_NEW_PROCESS_GROUP for proactor tests Dec 22, 2018

@asvetlov

This comment has been minimized.

Copy link
Contributor

asvetlov commented Dec 22, 2018

@njsmith what do you suggest?
Merge the PR or reimplement the test to follow your recommendations?

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Dec 23, 2018

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.

@vladima

This comment has been minimized.

Copy link
Contributor Author

vladima commented Dec 23, 2018

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.

@eryksun

This comment has been minimized.

Copy link

eryksun commented Dec 23, 2018

What I eventually realized is: normally, when a CTRL_*_EVENT arrives 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 intercepts CTRL_C_EVENT and calls raise(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()
@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Dec 23, 2018

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

@eryksun

This comment has been minimized.

Copy link

eryksun commented Dec 23, 2018

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.

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Dec 23, 2018

@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?

@vladima

This comment has been minimized.

Copy link
Contributor Author

vladima commented Dec 23, 2018

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')

@vladima vladima changed the title bpo-23057: Use CREATE_NEW_PROCESS_GROUP for proactor tests bpo-23057: Use 'raise' to emulate ctrl-c in proactor tests Dec 24, 2018

vladima added some commits Dec 24, 2018

switch ctrl-c test to use 'raise'. This uncovered an issue with missi…
…ng unregistration that was also fixed.
@vladima

This comment has been minimized.

Copy link
Contributor Author

vladima commented Dec 26, 2018

is there anything else that should be done in this PR?

@vladima

This comment has been minimized.

Copy link
Contributor Author

vladima commented Dec 27, 2018

@njsmith, @asvetlov - can you please tell if there is anything else that should be added/changed in this PR?

@eryksun

This comment has been minimized.

Copy link

eryksun commented Dec 28, 2018

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.

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Dec 28, 2018

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.

@jkloth

This comment has been minimized.

Copy link
Contributor

jkloth commented Dec 28, 2018

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.

@zooba

This comment has been minimized.

Copy link
Member

zooba commented Dec 30, 2018

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.

@vladima

This comment has been minimized.

Copy link
Contributor Author

vladima commented Jan 4, 2019

is there anything else that should be done for this PR?

@asvetlov

This comment has been minimized.

Copy link
Contributor

asvetlov commented Jan 5, 2019

I think everything is fine.
Thank you, @vladima

@asvetlov asvetlov merged commit 67ba547 into python:master Jan 5, 2019

5 checks passed

Azure Pipelines PR #20181224.21 succeeded
Details
bedevere/issue-number Issue number 23057 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 5, 2019

@asvetlov: Please replace # with GH- in the commit message next time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.