From f8d363822c9e3f9b3ee416e0f0e7343637517c01 Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Wed, 20 Jan 2021 14:27:28 -0800 Subject: [PATCH] Patch ceval to add callback to use for KeyboardInterrupt handling (#1148) * Patch ceval to add interrupt handler callback * Don't use SharedArrayBuffer in test, Firefox doesn't expose it * Fix test * Retest Co-authored-by: Jan Max Meyer --- Makefile | 17 +- .../patches/0001-Add-pyodide-callback.patch | 174 ++++++++++++++++++ src/core/keyboard_interrupt.c | 44 +++++ src/core/keyboard_interrupt.h | 7 + src/core/main.c | 2 + src/tests/test_pyodide.py | 25 +++ 6 files changed, 263 insertions(+), 6 deletions(-) create mode 100644 cpython/patches/0001-Add-pyodide-callback.patch create mode 100644 src/core/keyboard_interrupt.c create mode 100644 src/core/keyboard_interrupt.h diff --git a/Makefile b/Makefile index 2cc7346e2..c6dc09350 100644 --- a/Makefile +++ b/Makefile @@ -53,16 +53,21 @@ all: check \ echo -e "\nSUCCESS!" -build/pyodide.asm.js: src/core/main.o \ - src/core/jsproxy.o src/core/js2python.o \ +build/pyodide.asm.js: \ src/core/error_handling.o \ + src/core/hiwire.o \ + src/core/js2python.o \ + src/core/jsproxy.o \ + src/core/keyboard_interrupt.o \ + src/core/main.o \ src/core/pyproxy.o \ - src/core/python2js.o \ src/core/python2js_buffer.o \ - src/core/runpython.o src/core/hiwire.o \ - src/webbrowser.py \ - src/_testcapi.py \ + src/core/python2js.o \ + src/core/runpython.o \ src/pystone.py \ + src/_testcapi.py \ + src/webbrowser.py \ + \ $(wildcard src/pyodide-py/pyodide/*.py) \ $(CPYTHONLIB) date +"[%F %T] Building pyodide.asm.js..." diff --git a/cpython/patches/0001-Add-pyodide-callback.patch b/cpython/patches/0001-Add-pyodide-callback.patch new file mode 100644 index 000000000..321733afb --- /dev/null +++ b/cpython/patches/0001-Add-pyodide-callback.patch @@ -0,0 +1,174 @@ +From ff5dbb071a679ee1ea158feac363388e878ce402 Mon Sep 17 00:00:00 2001 +From: Hood +Date: Sat, 16 Jan 2021 11:54:57 -0800 +Subject: [PATCH] Add pyodide callback + +--- + Include/ceval.h | 1 + + Include/cpython/pystate.h | 3 ++ + Python/ceval.c | 60 +++++++++++++++++++++++---------------- + Python/pystate.c | 2 ++ + 4 files changed, 42 insertions(+), 24 deletions(-) + +This patch adds a callback called pyodide_callback with signature +`int callback(void)` to the main loop in `ceval.c`. This function gets called once +per opcode except after opcodes `SETUP_FINALLY`, `SETUP_WITH`, `BEFORE_ASYNC_WITH`, +and `YIELD_FROM`. The main loop normally prevents signal handling, etc from happening +after these instructions, so I figured this should apply to my callback too. +(There is an extra layer of protection here though because in the callback we use +`PyErr_SetInterrupt` which simulates a SIGINT signal and triggers the KeyboardInterrupt +via the standard mechanism.) + +Note that we call the callback outside of the normal +`if (_Py_atomic_load_relaxed(eval_breaker))` +block where most of the "periodic things" happen. This is because normally when a +"periodic thing" is queued, the `eval_breaker` flag is set signalling the need for +handling. The whole point of this patch though is that we need to be able to set an +interrupt from a remote thread and the `eval_breaker` flag lives on the WASM heap. +Unless the whole WASM heap is made into a `SharedArrayBuffer` and shared between +webworkers, we have no way to set the `eval_breaker` flag. We still want to skip +the callback after the sensitive opcodes `SETUP_FINALLY`, `SETUP_WITH`, +`BEFORE_ASYNC_WITH`, and `YIELD_FROM`, so we hoisted the check for that condition +out of the `eval_breaker` conditional. + +We also patched the `threadstate` struct to include a `pyodide_callback` field, patch +`new_threadstate` to initialize the `pyodide_callback` field to `NULL`, and add a new +API `PyPyodide_SetPyodideCallback` to set `pyodide_callback`. This makes +`PyPyodide_SetPyodideCallback` behave much like `PyEval_SetTrace`, except lighter +weight. + +diff --git a/Include/ceval.h b/Include/ceval.h +index 36fd014..5f5fbc4 100644 +--- a/Include/ceval.h ++++ b/Include/ceval.h +@@ -31,6 +31,7 @@ PyAPI_FUNC(PyObject *) PyEval_CallMethod(PyObject *obj, + #ifndef Py_LIMITED_API + PyAPI_FUNC(void) PyEval_SetProfile(Py_tracefunc, PyObject *); + PyAPI_FUNC(void) PyEval_SetTrace(Py_tracefunc, PyObject *); ++PyAPI_FUNC(void) PyPyodide_SetPyodideCallback(PyPyodide_callback); + PyAPI_FUNC(void) _PyEval_SetCoroutineOriginTrackingDepth(int new_depth); + PyAPI_FUNC(int) _PyEval_GetCoroutineOriginTrackingDepth(void); + PyAPI_FUNC(void) _PyEval_SetAsyncGenFirstiter(PyObject *); +diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h +index 94b0809..b215056 100644 +--- a/Include/cpython/pystate.h ++++ b/Include/cpython/pystate.h +@@ -17,6 +17,7 @@ PyAPI_FUNC(PyObject *) _PyInterpreterState_GetMainModule(PyInterpreterState *); + + /* Py_tracefunc return -1 when raising an exception, or 0 for success. */ + typedef int (*Py_tracefunc)(PyObject *, struct _frame *, int, PyObject *); ++typedef int (*PyPyodide_callback)(void); + + /* The following values are used for 'what' for tracefunc functions + * +@@ -73,6 +74,8 @@ struct _ts { + Py_tracefunc c_tracefunc; + PyObject *c_profileobj; + PyObject *c_traceobj; ++ ++ PyPyodide_callback pyodide_callback; + + /* The exception currently being raised */ + PyObject *curexc_type; +diff --git a/Python/ceval.c b/Python/ceval.c +index 3306fb9..86e7639 100644 +--- a/Python/ceval.c ++++ b/Python/ceval.c +@@ -1198,31 +1198,31 @@ main_loop: + async I/O handler); see Py_AddPendingCall() and + Py_MakePendingCalls() above. */ + +- if (_Py_atomic_load_relaxed(eval_breaker)) { +- opcode = _Py_OPCODE(*next_instr); +- if (opcode == SETUP_FINALLY || +- opcode == SETUP_WITH || +- opcode == BEFORE_ASYNC_WITH || +- opcode == YIELD_FROM) { +- /* Few cases where we skip running signal handlers and other +- pending calls: +- - If we're about to enter the 'with:'. It will prevent +- emitting a resource warning in the common idiom +- 'with open(path) as file:'. +- - If we're about to enter the 'async with:'. +- - If we're about to enter the 'try:' of a try/finally (not +- *very* useful, but might help in some cases and it's +- traditional) +- - If we're resuming a chain of nested 'yield from' or +- 'await' calls, then each frame is parked with YIELD_FROM +- as its next opcode. If the user hit control-C we want to +- wait until we've reached the innermost frame before +- running the signal handler and raising KeyboardInterrupt +- (see bpo-30039). +- */ +- goto fast_next_opcode; +- } ++ opcode = _Py_OPCODE(*next_instr); ++ if (opcode == SETUP_FINALLY || ++ opcode == SETUP_WITH || ++ opcode == BEFORE_ASYNC_WITH || ++ opcode == YIELD_FROM) { ++ /* Few cases where we skip running signal handlers and other ++ pending calls: ++ - If we're about to enter the 'with:'. It will prevent ++ emitting a resource warning in the common idiom ++ 'with open(path) as file:'. ++ - If we're about to enter the 'async with:'. ++ - If we're about to enter the 'try:' of a try/finally (not ++ *very* useful, but might help in some cases and it's ++ traditional) ++ - If we're resuming a chain of nested 'yield from' or ++ 'await' calls, then each frame is parked with YIELD_FROM ++ as its next opcode. If the user hit control-C we want to ++ wait until we've reached the innermost frame before ++ running the signal handler and raising KeyboardInterrupt ++ (see bpo-30039). ++ */ ++ goto fast_next_opcode; ++ } + ++ if (_Py_atomic_load_relaxed(eval_breaker)) { + if (_Py_atomic_load_relaxed(&ceval->signals_pending)) { + if (handle_signals(runtime) != 0) { + goto error; +@@ -1262,6 +1262,12 @@ main_loop: + goto error; + } + } ++ ++ if(tstate->pyodide_callback != NULL){ ++ if(tstate->pyodide_callback() != 0){ ++ goto error; ++ } ++ } + + fast_next_opcode: + f->f_lasti = INSTR_OFFSET(); +@@ -4727,6 +4733,12 @@ PyEval_SetTrace(Py_tracefunc func, PyObject *arg) + || (tstate->c_profilefunc != NULL)); + } + ++void ++PyPyodide_SetPyodideCallback(PyPyodide_callback pyodide_callback){ ++ PyThreadState *tstate = _PyThreadState_GET(); ++ tstate->pyodide_callback = pyodide_callback; ++} ++ + void + _PyEval_SetCoroutineOriginTrackingDepth(int new_depth) + { +diff --git a/Python/pystate.c b/Python/pystate.c +index aba673c..b809208 100644 +--- a/Python/pystate.c ++++ b/Python/pystate.c +@@ -592,6 +592,8 @@ new_threadstate(PyInterpreterState *interp, int init) + tstate->c_tracefunc = NULL; + tstate->c_profileobj = NULL; + tstate->c_traceobj = NULL; ++ ++ tstate->pyodide_callback = NULL; + + tstate->trash_delete_nesting = 0; + tstate->trash_delete_later = NULL; +-- +2.17.1 + diff --git a/src/core/keyboard_interrupt.c b/src/core/keyboard_interrupt.c new file mode 100644 index 000000000..f1d57f755 --- /dev/null +++ b/src/core/keyboard_interrupt.c @@ -0,0 +1,44 @@ +#define PY_SSIZE_T_CLEAN +#include "Python.h" + +#include "keyboard_interrupt.h" +#include + +static int callback_clock = 1000; + +int +pyodide_callback(void) +{ + callback_clock--; + if (callback_clock == 0) { + callback_clock = 1000; + int interrupt_buffer = EM_ASM_INT({ + let result = Module.interrupt_buffer[0]; + Module.interrupt_buffer[0] = 0; + return result; + }); + if (interrupt_buffer == 2) { + PyErr_SetInterrupt(); + } + } + return 0; +} + +int +keyboard_interrupt_init() +{ + EM_ASM( + { + Module.setInterruptBuffer = function(buffer) + { + Module.interrupt_buffer = buffer; + if (buffer) { + _PyPyodide_SetPyodideCallback($0); + } else { + _PyPyodide_SetPyodideCallback(0); + } + }; + }, + pyodide_callback); + return 0; +} diff --git a/src/core/keyboard_interrupt.h b/src/core/keyboard_interrupt.h new file mode 100644 index 000000000..01d0fb13e --- /dev/null +++ b/src/core/keyboard_interrupt.h @@ -0,0 +1,7 @@ +#ifndef KEYBOARD_INTERRUPT_H +#define KEYBOARD_INTERRUPT_H + +int +keyboard_interrupt_init(); + +#endif /* KEYBOARD_INTERRUPT_H */ diff --git a/src/core/main.c b/src/core/main.c index f278abde0..7e338d876 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -8,6 +8,7 @@ #include "hiwire.h" #include "js2python.h" #include "jsproxy.h" +#include "keyboard_interrupt.h" #include "pyproxy.h" #include "python2js.h" #include "runpython.h" @@ -98,6 +99,7 @@ main(int argc, char** argv) TRY_INIT_WITH_CORE_MODULE(JsProxy); // JsProxy needs to be before JsImport TRY_INIT(pyproxy); TRY_INIT(python2js); + TRY_INIT(keyboard_interrupt); PyObject* module_dict = PyImport_GetModuleDict(); // borrowed if (PyDict_SetItemString(module_dict, "_pyodide_core", core_module)) { diff --git a/src/tests/test_pyodide.py b/src/tests/test_pyodide.py index 9ce71a081..22d6a9725 100644 --- a/src/tests/test_pyodide.py +++ b/src/tests/test_pyodide.py @@ -192,3 +192,28 @@ def test_hiwire_is_promise(selenium): return pyodide._module.hiwire.isPromise(pyodide.globals); """ ) + + +def test_keyboard_interrupt(selenium): + assert selenium.run_js( + """ + x = new Int8Array(1) + pyodide._module.setInterruptBuffer(x) + window.triggerKeyboardInterrupt = function(){ + x[0] = 2; + } + try { + pyodide.runPython(` + from js import triggerKeyboardInterrupt + x = 0 + while True: + x += 1 + if x == 2000: + triggerKeyboardInterrupt() + `) + } catch(e){} + return pyodide.runPython(` + 2000 < x < 2500 + `) + """ + )