Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
FIX: Add PyOS_InputHook back to macos backend#26970
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ksunden left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can confirm that this works.
My objective C is a bit rusty, but nothing looks off to me.
I could also go either way on the solution for 3.8, but perhaps a slight leaning for just backporting this, but not super strong in that opinion
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need to check for an existing signal handler (set by Python for example)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 theinput block I think so it needs either anenter orctrl+c to kill that one again. Keeping theraise 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would guess something similar to the SIGINT handling in Qt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. Anotherctrl+c exits the program with aKeyboardInterrupt.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: initializedOut of all of these, I'd say that themacosx state in this PR is the most ideal...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@anntzer pinging you here in case you have thoughts on this PR and the current status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 waitingon standard input from the user. This means we should flush ourevents while waiting for any input from the user so the figuresare responsive.
tacaswell commentedOct 17, 2023
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: fromPyQt6.QtWidgetsimportQApplication,QWidget,QLabel,QPushButtonapp=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: ") |
tacaswell commentedOct 25, 2023
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). |
tacaswell commentedOct 25, 2023
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). |
greglucas commentedOct 26, 2023
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, 0x0000000000000000Thread 0 Crashed:0 libsystem_kernel.dylib 0x7ff8019071f2 0x7ff8018ff000 + 332661 libsystem_c.dylib 0x7ff801865b45 0x7ff8017e6000 + 5230772 QtCore 0x11c312289qAbort() + 93 QtCore 0x11c315939 0x11c309000 + 515134 QtCore 0x11c6815ba QMessageLogger::fatal(char const*, ...) const + 1725 QtCore.abi3.so 0x11c0c2ca6pyqt6_err_print() + 7586 sip.cpython-310-darwin.so 0x10d13b306 sip_api_call_procedure_method + 2467 QtWidgets.abi3.so 0x11ca20248 sipQWidget::paintEvent(QPaintEvent*) + 1048 QtWidgets 0x12667305e QWidget::event(QEvent*) + 12629 QtWidgets.abi3.so 0x11ca2110f sipQWidget::event(QEvent*) + 19110 QtWidgets 0x1266239e7 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 24711 QtWidgets 0x12662480c QApplication::notify(QObject*, QEvent*) + 50812 QtWidgets.abi3.so 0x11ca0bec6 sipQApplication::notify(QObject*, QEvent*) + 23013 QtCore 0x11c37723a QCoreApplication::notifyInternal2(QObject*, QEvent*) + 17014 QtWidgets 0x1266651bd QWidgetPrivate::drawWidget(QPaintDevice*, QRegion const&, QPoint const&, QFlags<QWidgetPrivate::DrawWidgetFlag>, QPainter*, QWidgetRepaintManager*) + 398115 QtWidgets 0x12668514eQWidgetRepaintManager::paintAndFlush() + 470216 QtWidgets 0x1266833db QWidgetRepaintManager::sync(QWidget*, QRegion const&) + 61917 QtWidgets 0x12668b7c6 0x126616000 + 48122218 QtWidgets 0x126688a69 0x126616000 + 46960919 QtWidgets 0x1266239e7 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 24720 QtWidgets 0x12662480c QApplication::notify(QObject*, QEvent*) + 50821 QtWidgets.abi3.so 0x11ca0bec6 sipQApplication::notify(QObject*, QEvent*) + 23022 QtCore 0x11c37723a QCoreApplication::notifyInternal2(QObject*, QEvent*) + 17023 QtGui 0x125e98cd6 QGuiApplicationPrivate::processExposeEvent(QWindowSystemInterfacePrivate::ExposeEvent*) + 40624 QtGui 0x125ef3833 0x125e0c000 + 94827525 QtGui 0x125ef0029 bool QWindowSystemInterface::handleExposeEvent<QWindowSystemInterface::SynchronousDelivery>(QWindow*, QRegion const&) + 5726 libqcocoa.dylib 0x11ced8cba 0x11ce95000 + 27769027 libqcocoa.dylib 0x11ceef5b7 0x11ce95000 + 37010328 AppKit 0x7ff804bfad79 0x7ff804a6f000 + 162136929 AppKit 0x7ff804b73454 0x7ff804a6f000 + 106606830 QuartzCore 0x7ff8092dc0c0 0x7ff8092bb000 + 13536031 QuartzCore 0x7ff80946318b 0x7ff8092bb000 + 173709932 QuartzCore 0x7ff8092bcff3 0x7ff8092bb000 + 817933 AppKit 0x7ff804c0bfcf 0x7ff804a6f000 + 169159934 AppKit 0x7ff80541ee90 0x7ff804a6f000 + 1015771235 CoreFoundation 0x7ff801a1a444 0x7ff80199e000 + 50899636 CoreFoundation 0x7ff801a1a36b 0x7ff80199e000 + 50877937 CoreFoundation 0x7ff801a198f6 0x7ff80199e000 + 50610238 CoreFoundation 0x7ff801a18f31 0x7ff80199e000 + 50360139 HIToolbox 0x7ff80b494dad 0x7ff80b466000 + 19191740 HIToolbox 0x7ff80b494bbe 0x7ff80b466000 + 19142241 HIToolbox 0x7ff80b494918 0x7ff80b466000 + 19074442 AppKit 0x7ff804aad5d0 0x7ff804a6f000 + 25544043 AppKit 0x7ff804aac47a 0x7ff804a6f000 + 25100244 AppKit 0x7ff804a9eae8 0x7ff804a6f000 + 19530445 libqcocoa.dylib 0x11ceac577 0x11ce95000 + 9560746 QtCore 0x11c380a46 QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 48647 QtCore.abi3.so 0x11bf9cfe1qtcore_input_hook() + 14548 Python 0x108216edc my_fgets + 4449 Python 0x108216d21 PyOS_StdioReadline + 17750 Python 0x108217101 PyOS_Readline + 20951 Python 0x1083775a3 builtin_input_impl + 216352 Python 0x1082afd36 cfunction_vectorcall_FASTCALL + 8653 Python 0x108389fbf call_function + 17554 Python 0x10837f8dd _PyEval_EvalFrameDefault + 2039755 Python 0x108378fe2 _PyEval_Vector + 40256 Python 0x1083ee14d pyrun_file + 33357 Python 0x1083ed90d _PyRun_SimpleFileObject + 36558 Python 0x1083ecf5f _PyRun_AnyFileObject + 14359 Python 0x108418da7 pymain_run_file_obj + 19960 Python 0x108418575 pymain_run_file + 8561 Python 0x108417cfe pymain_run_python + 33462 Python 0x108417b67 Py_RunMain + 2363 Python 0x108418f42 pymain_main + 5064 Python 0x1084191ea Py_BytesMain + 4265 dyld 0x7ff8015e541f 0x7ff8015df000 + 25631 TkException Type: EXC_CRASH (SIGABRT)Exception Codes: 0x0000000000000000, 0x0000000000000000Thread 0 Crashed:0 libsystem_kernel.dylib 0x7ff8019071f2 0x7ff8018ff000 + 332661 libsystem_c.dylib 0x7ff801865b45 0x7ff8017e6000 + 5230772 Python 0x1049c048d fatal_error_exit + 133 Python 0x1049c02ff fatal_error + 314 Python 0x1049c33e4 _Py_FatalErrorFunc + 525 Python 0x104951b40 _Py_FatalError_TstateNULL + 166 Python 0x10495256c PyEval_RestoreThread + 607 _tkinter.cpython-310-darwin.so 0x1096f03df PythonCmd + 958 libtcl8.6.dylib 0x11814ac71 TclNRRunCallbacks + 799 libtcl8.6.dylib 0x11814bdc4 TclEvalEx + 185610 libtcl8.6.dylib 0x11814b67e Tcl_EvalEx + 2611 libtk8.6.dylib 0x117e74662 Tk_BindEvent + 562612 libtk8.6.dylib 0x117e7a3c1 TkBindEventProc + 33513 libtk8.6.dylib 0x117e819f4 Tk_HandleEvent + 83914 libtk8.6.dylib 0x117e9c755 Tk_DestroyWindow + 49215 libtk8.6.dylib 0x117e9c698 Tk_DestroyWindow + 30316 libtk8.6.dylib 0x117e9e616 DeleteWindowsExitProc + 15217 libtk8.6.dylib 0x117e82317 TkFinalizeThread + 9518 libtcl8.6.dylib 0x1181bc882 FinalizeThread + 6119 libtcl8.6.dylib 0x1181bc6a3 Tcl_Exit + 9820 libtk8.6.dylib 0x117f2a97e TkMacOSXSignalHandler + 2521 libsystem_platform.dylib 0x7ff80196c5ed 0x7ff801969000 + 1380522??? 0x7ff84506da30???23 _tkinter.cpython-310-darwin.so 0x1096f2bc2 EventHook + 22624 Python 0x1047f0edc my_fgets + 4425 Python 0x1047f0d21 PyOS_StdioReadline + 17726 Python 0x1047f1101 PyOS_Readline + 20927 Python 0x1049515a3 builtin_input_impl + 216328 Python 0x104889d36 cfunction_vectorcall_FASTCALL + 8629 Python 0x104963fbf call_function + 17530 Python 0x1049598dd _PyEval_EvalFrameDefault + 2039731 Python 0x104952fe2 _PyEval_Vector + 40232 Python 0x1049c814d pyrun_file + 33333 Python 0x1049c790d _PyRun_SimpleFileObject + 36534 Python 0x1049c6f5f _PyRun_AnyFileObject + 14335 Python 0x1049f2da7 pymain_run_file_obj + 19936 Python 0x1049f2575 pymain_run_file + 8537 Python 0x1049f1cfe pymain_run_python + 33438 Python 0x1049f1b67 Py_RunMain + 2339 Python 0x1049f2f42 pymain_main + 5040 Python 0x1049f31ea Py_BytesMain + 4241 dyld 0x7ff8015e541f 0x7ff8015df000 + 25631 |
tacaswell commentedOct 26, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Never mind, I created a new shell and did not have this problem....clearly something very weird locally! |
…970-on-v3.8.xBackport 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 commitec6717a 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