FIX: Add PyOS_InputHook back to macos backend #26970
Conversation
src/_macosx.m
Outdated
| // Set up a SIGINT handler to interrupt the event loop if ctrl+c comes in too | ||
| struct sigaction action = {0}; | ||
| action.sa_handler = handleSigint; | ||
| sigaction(SIGINT, &action, NULL); |
There was a problem hiding this comment.
Do we need to check for an existing signal handler (set by Python for example)?
There was a problem hiding this comment.
I added that capability in, and now the code requires two ctrl+c to exit. The first one only exits the event loop, but then we are still waiting for the input block I think so it needs either an enter or ctrl+c to kill that one again. Keeping the raise SIGINT in there didn't matter one way or the other.
I'm not sure what is preferable. Setting it to the default sigint handler kills the process immediately. But I suppose this is OK as long as you know you need to kills sent to exit? I am not sure about how this is typically handled so any advice/suggestions are welcome.
There was a problem hiding this comment.
I would guess something similar to the SIGINT handling in Qt?
There was a problem hiding this comment.
Is there a reference implementation for that? I'm still not following what the expected/desired path is compared to what we currently have.
Just to clarify the current situation with that example code and then going to the terminal that is waiting for [enter] input.
macosx: ctrl+c -> leaves the event loop, then waits for the input still. Another ctrl+c exits the program with a KeyboardInterrupt.
qtagg: ctrl+c -> does nothing while waiting for the input, so hitting it multiple times does not kill the program. Clicking on the figure window again (bouncing the event loop I guess?) kills the program ungracefully with an abort.
tkagg: ctrl+c -> kills the program with a fatal Python error
Fatal Python error: PyEval_RestoreThread: the function must be called with the GIL held, but the GIL is released (the current Python thread state is NULL)
Python runtime state: initialized
Out of all of these, I'd say that the macosx state in this PR is the most ideal...
There was a problem hiding this comment.
@anntzer pinging you here in case you have thoughts on this PR and the current status
There was a problem hiding this comment.
Noted, but I cannot guarantee any amount of time to look at this in depth at least until the end of the week (for sure) and possibly for even longer.
The PyOS_InputHook is used to run our event loop while waiting on standard input from the user. This means we should flush our events while waiting for any input from the user so the figures are responsive.
|
Sorry this got long and rambling as I tried to sort out what is/should be going on. The sigint handling in the Qt backend is aimed at when we are blocking. If we are in the input hook we are a bit at the mercy of what upstream of the gui toolkit is doing and integration with readline. Despite my earlier comments, setting My knee-jerk thought as to what the behaviour should be is that ctrl-c at a blocking input with window open should forward that signal to the input so that it behaves like what happens when the input hook is not installed (the
input('bar: ')It looks like in the case of both Qt and tk if you mouse back over the figure it does deliver the I think this is because:
I got two different failures with tk, but then when I went to try and reproduce them I could not and am getting the behavior Greg described for OSX (first ctrl-c (plus an interaction with the figure) kills the event loop, the second behaves as expected Given that this whole artifice exists to support tk in CPython I'm inclined to say that should be the reference implemenation This is what just PyQt6 does: from PyQt6.QtWidgets import QApplication, QWidget, QLabel, QPushButton
app = QApplication(sys.argv)
widget = QWidget()
textLabel = QLabel(widget)
textLabel.setText("Hello World!")
textLabel.move(110, 85)
widget.setWindowTitle("PyQt6 Example")
button1 = QPushButton(widget)
button1.setText("Button1")
button1.move(64, 32)
button1.clicked.connect(lambda: print("hi bob"))
widget.show()
input("hit enter: ") |
|
On a bit more consideration, I do not think we should hold this on the exact details of the SIGINT handling. Worst case it is inconsistent with the other GUI frameworks, but that is still better than the current state. I also stand by the idea that we should install the input hook here (but not in any other case) because there is no "upstream" we can expect to do it for us (unlike pyside). |
|
Testing this I'm seeing that while the event loop is running, typing at stdin does not seem to stop it (but two ctrl-c will). I'm using "Terminal" and zsh (the out of the box defaults I think). |
|
Thanks for the detailed write-up, I think I get the same behavior as you describe. I'm not sure I follow:
The current example is waiting for "enter", so I don't think it should stop anything until that key is pressed. To summarize this PR: It adds back the ability to run the event loop while waiting for stdin. There is a case where a user hits ctrl+c to try and exit the program while waiting for sdtin. That case does not currently exit on any backend and you either need to run the event loop (moving mouse over figure) or press ctrl+c again. I think this case can be opened in a separate issue if we want to track it and try to improve it for all backends, but I'm not sure it is release-critical to hold this up. I'll include the full tracebacks here as well for reference: QtException Type: EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000
Thread 0 Crashed:
0 libsystem_kernel.dylib 0x7ff8019071f2 0x7ff8018ff000 + 33266
1 libsystem_c.dylib 0x7ff801865b45 0x7ff8017e6000 + 523077
2 QtCore 0x11c312289 qAbort() + 9
3 QtCore 0x11c315939 0x11c309000 + 51513
4 QtCore 0x11c6815ba QMessageLogger::fatal(char const*, ...) const + 172
5 QtCore.abi3.so 0x11c0c2ca6 pyqt6_err_print() + 758
6 sip.cpython-310-darwin.so 0x10d13b306 sip_api_call_procedure_method + 246
7 QtWidgets.abi3.so 0x11ca20248 sipQWidget::paintEvent(QPaintEvent*) + 104
8 QtWidgets 0x12667305e QWidget::event(QEvent*) + 1262
9 QtWidgets.abi3.so 0x11ca2110f sipQWidget::event(QEvent*) + 191
10 QtWidgets 0x1266239e7 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 247
11 QtWidgets 0x12662480c QApplication::notify(QObject*, QEvent*) + 508
12 QtWidgets.abi3.so 0x11ca0bec6 sipQApplication::notify(QObject*, QEvent*) + 230
13 QtCore 0x11c37723a QCoreApplication::notifyInternal2(QObject*, QEvent*) + 170
14 QtWidgets 0x1266651bd QWidgetPrivate::drawWidget(QPaintDevice*, QRegion const&, QPoint const&, QFlags<QWidgetPrivate::DrawWidgetFlag>, QPainter*, QWidgetRepaintManager*) + 3981
15 QtWidgets 0x12668514e QWidgetRepaintManager::paintAndFlush() + 4702
16 QtWidgets 0x1266833db QWidgetRepaintManager::sync(QWidget*, QRegion const&) + 619
17 QtWidgets 0x12668b7c6 0x126616000 + 481222
18 QtWidgets 0x126688a69 0x126616000 + 469609
19 QtWidgets 0x1266239e7 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 247
20 QtWidgets 0x12662480c QApplication::notify(QObject*, QEvent*) + 508
21 QtWidgets.abi3.so 0x11ca0bec6 sipQApplication::notify(QObject*, QEvent*) + 230
22 QtCore 0x11c37723a QCoreApplication::notifyInternal2(QObject*, QEvent*) + 170
23 QtGui 0x125e98cd6 QGuiApplicationPrivate::processExposeEvent(QWindowSystemInterfacePrivate::ExposeEvent*) + 406
24 QtGui 0x125ef3833 0x125e0c000 + 948275
25 QtGui 0x125ef0029 bool QWindowSystemInterface::handleExposeEvent<QWindowSystemInterface::SynchronousDelivery>(QWindow*, QRegion const&) + 57
26 libqcocoa.dylib 0x11ced8cba 0x11ce95000 + 277690
27 libqcocoa.dylib 0x11ceef5b7 0x11ce95000 + 370103
28 AppKit 0x7ff804bfad79 0x7ff804a6f000 + 1621369
29 AppKit 0x7ff804b73454 0x7ff804a6f000 + 1066068
30 QuartzCore 0x7ff8092dc0c0 0x7ff8092bb000 + 135360
31 QuartzCore 0x7ff80946318b 0x7ff8092bb000 + 1737099
32 QuartzCore 0x7ff8092bcff3 0x7ff8092bb000 + 8179
33 AppKit 0x7ff804c0bfcf 0x7ff804a6f000 + 1691599
34 AppKit 0x7ff80541ee90 0x7ff804a6f000 + 10157712
35 CoreFoundation 0x7ff801a1a444 0x7ff80199e000 + 508996
36 CoreFoundation 0x7ff801a1a36b 0x7ff80199e000 + 508779
37 CoreFoundation 0x7ff801a198f6 0x7ff80199e000 + 506102
38 CoreFoundation 0x7ff801a18f31 0x7ff80199e000 + 503601
39 HIToolbox 0x7ff80b494dad 0x7ff80b466000 + 191917
40 HIToolbox 0x7ff80b494bbe 0x7ff80b466000 + 191422
41 HIToolbox 0x7ff80b494918 0x7ff80b466000 + 190744
42 AppKit 0x7ff804aad5d0 0x7ff804a6f000 + 255440
43 AppKit 0x7ff804aac47a 0x7ff804a6f000 + 251002
44 AppKit 0x7ff804a9eae8 0x7ff804a6f000 + 195304
45 libqcocoa.dylib 0x11ceac577 0x11ce95000 + 95607
46 QtCore 0x11c380a46 QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 486
47 QtCore.abi3.so 0x11bf9cfe1 qtcore_input_hook() + 145
48 Python 0x108216edc my_fgets + 44
49 Python 0x108216d21 PyOS_StdioReadline + 177
50 Python 0x108217101 PyOS_Readline + 209
51 Python 0x1083775a3 builtin_input_impl + 2163
52 Python 0x1082afd36 cfunction_vectorcall_FASTCALL + 86
53 Python 0x108389fbf call_function + 175
54 Python 0x10837f8dd _PyEval_EvalFrameDefault + 20397
55 Python 0x108378fe2 _PyEval_Vector + 402
56 Python 0x1083ee14d pyrun_file + 333
57 Python 0x1083ed90d _PyRun_SimpleFileObject + 365
58 Python 0x1083ecf5f _PyRun_AnyFileObject + 143
59 Python 0x108418da7 pymain_run_file_obj + 199
60 Python 0x108418575 pymain_run_file + 85
61 Python 0x108417cfe pymain_run_python + 334
62 Python 0x108417b67 Py_RunMain + 23
63 Python 0x108418f42 pymain_main + 50
64 Python 0x1084191ea Py_BytesMain + 42
65 dyld 0x7ff8015e541f 0x7ff8015df000 + 25631TkException Type: EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000
Thread 0 Crashed:
0 libsystem_kernel.dylib 0x7ff8019071f2 0x7ff8018ff000 + 33266
1 libsystem_c.dylib 0x7ff801865b45 0x7ff8017e6000 + 523077
2 Python 0x1049c048d fatal_error_exit + 13
3 Python 0x1049c02ff fatal_error + 31
4 Python 0x1049c33e4 _Py_FatalErrorFunc + 52
5 Python 0x104951b40 _Py_FatalError_TstateNULL + 16
6 Python 0x10495256c PyEval_RestoreThread + 60
7 _tkinter.cpython-310-darwin.so 0x1096f03df PythonCmd + 95
8 libtcl8.6.dylib 0x11814ac71 TclNRRunCallbacks + 79
9 libtcl8.6.dylib 0x11814bdc4 TclEvalEx + 1856
10 libtcl8.6.dylib 0x11814b67e Tcl_EvalEx + 26
11 libtk8.6.dylib 0x117e74662 Tk_BindEvent + 5626
12 libtk8.6.dylib 0x117e7a3c1 TkBindEventProc + 335
13 libtk8.6.dylib 0x117e819f4 Tk_HandleEvent + 839
14 libtk8.6.dylib 0x117e9c755 Tk_DestroyWindow + 492
15 libtk8.6.dylib 0x117e9c698 Tk_DestroyWindow + 303
16 libtk8.6.dylib 0x117e9e616 DeleteWindowsExitProc + 152
17 libtk8.6.dylib 0x117e82317 TkFinalizeThread + 95
18 libtcl8.6.dylib 0x1181bc882 FinalizeThread + 61
19 libtcl8.6.dylib 0x1181bc6a3 Tcl_Exit + 98
20 libtk8.6.dylib 0x117f2a97e TkMacOSXSignalHandler + 25
21 libsystem_platform.dylib 0x7ff80196c5ed 0x7ff801969000 + 13805
22 ??? 0x7ff84506da30 ???
23 _tkinter.cpython-310-darwin.so 0x1096f2bc2 EventHook + 226
24 Python 0x1047f0edc my_fgets + 44
25 Python 0x1047f0d21 PyOS_StdioReadline + 177
26 Python 0x1047f1101 PyOS_Readline + 209
27 Python 0x1049515a3 builtin_input_impl + 2163
28 Python 0x104889d36 cfunction_vectorcall_FASTCALL + 86
29 Python 0x104963fbf call_function + 175
30 Python 0x1049598dd _PyEval_EvalFrameDefault + 20397
31 Python 0x104952fe2 _PyEval_Vector + 402
32 Python 0x1049c814d pyrun_file + 333
33 Python 0x1049c790d _PyRun_SimpleFileObject + 365
34 Python 0x1049c6f5f _PyRun_AnyFileObject + 143
35 Python 0x1049f2da7 pymain_run_file_obj + 199
36 Python 0x1049f2575 pymain_run_file + 85
37 Python 0x1049f1cfe pymain_run_python + 334
38 Python 0x1049f1b67 Py_RunMain + 23
39 Python 0x1049f2f42 pymain_main + 50
40 Python 0x1049f31ea Py_BytesMain + 42
41 dyld 0x7ff8015e541f 0x7ff8015df000 + 25631 |
|
Never mind, I created a new shell and did not have this problem....clearly something very weird locally! |
…970-on-v3.8.x Backport PR #26970 on branch v3.8.x (FIX: Add PyOS_InputHook back to macos backend)
PR summary
The
PyOS_InputHookis used to run our event loop while waiting on standard input from the user. This means we should flush our events while waiting for any input from the user so the figures are responsive.Rather than reverting the cleanup commit ec6717a I tried to rewrite that in the newer NS framework utilities instead of all of the CF work that was being done. I added comments along the way indicating what is actually happening for the next time someone has to dig into this.
Test-case from the linked Issue:
closes #26869
Note: I'm not sure if we want to backport this as a bugfix since it is a bit of a rewrite, or would we rather just revert that other commit on the 3.8.x branch and keep this moving forward on main?
PR checklist