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 <jmm@phorward.de>

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..."

cpython/patches/0001-Add-pyodide-callback.patch

@@ -0,0 +1,174 @@
+From ff5dbb071a679ee1ea158feac363388e878ce402 Mon Sep 17 00:00:00 2001
+From: Hood <hood@mit.edu>
+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
+

src/core/keyboard_interrupt.c

@@ -0,0 +1,44 @@
+#define PY_SSIZE_T_CLEAN
+#include "Python.h"
+
+#include "keyboard_interrupt.h"
+#include <emscripten.h>
+
+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;
+}

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 */

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

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
+        `)
+        """
+    )