Give up the GIL during Starlark execution (#28)

* Give up the GIL during Starlark execution

I had some weird ideas about the GIL that I cleared up today. This
removes an unneccesary GIL lock in one spot, and changes `exec` and
`eval` to give up the GIL while they're busy executing Starlark code.
A mutex is added per Starlark thread to prevent multiple Python threads
from trying to access the same Starlark thread at once.

Also removed a couple unneccesary Cgo wrappers where I could use the
Python API directly instead.

* Remove another unused wrapper

starlark.c

@@ -18,12 +18,16 @@ static char *exec_keywords[] = {"defs", "filename", NULL};
 /* Helpers to parse method arguments */
 int CgoParseEvalArgs(PyObject *args, PyObject *kwargs, char **expr,
                      char **filename, unsigned int *parse) {
+  /* Necessary because Cgo can't do varargs */
+  /* One required string, folloed by an optional string and an optional bool */
   return PyArg_ParseTupleAndKeywords(args, kwargs, "s|$sp", eval_keywords, expr,
                                      filename, parse);
 }
 
 int GgoParseExecArgs(PyObject *args, PyObject *kwargs, char **defs,
                      char **filename) {
+  /* Necessary because Cgo can't do varargs */
+  /* One required string, folloed by an optional string */
   return PyArg_ParseTupleAndKeywords(args, kwargs, "s|$s", exec_keywords, defs,
                                      filename);
 }
@@ -65,59 +69,65 @@ static PyModuleDef pystarlark_lib = {
 
 /* Helpers for Cgo to build exception arguments */
 PyObject *CgoStarlarkErrorArgs(const char *error_msg, const char *error_type) {
+  /* Necessary because Cgo can't do varargs */
   return Py_BuildValue("ss", error_msg, error_type);
 }
 
 PyObject *CgoSyntaxErrorArgs(const char *error_msg, const char *error_type,
                              const char *msg, const char *filename,
                              const unsigned int line,
                              const unsigned int column) {
+  /* Necessary because Cgo can't do varargs */
+  /* Four strings and two unsigned integers */
   return Py_BuildValue("ssssII", error_msg, error_type, msg, filename, line,
                        column);
 }
 
 PyObject *CgoEvalErrorArgs(const char *error_msg, const char *error_type,
                            const char *backtrace) {
+  /* Necessary because Cgo can't do varargs */
+  /* Three strings */
   return Py_BuildValue("sss", error_msg, error_type, backtrace);
 }
 
 PyObject *CgoResolveErrorItem(const char *msg, const unsigned int line,
                               const unsigned int column) {
+  /* Necessary because Cgo can't do varargs */
+  /* A string and two unsigned integers */
   PyObject *args = Py_BuildValue("sII", msg, line, column);
-  PyGILState_STATE gilstate = PyGILState_Ensure();
   PyObject *obj = PyObject_CallObject(ResolveErrorItem, args);
-  PyGILState_Release(gilstate);
-  Py_XDECREF(args);
-
+  Py_DECREF(args);
   return obj;
 }
 
 PyObject *CgoResolveErrorArgs(const char *error_msg, const char *error_type,
                               PyObject *errors) {
+  /* Necessary because Cgo can't do varargs */
+  /* Two strings and a Python object */
   return Py_BuildValue("ssO", error_msg, error_type, errors);
 }
-/* Other assorted helpers for Cgo, which can't handle varargs or macros */
+
+/* Other assorted helpers for Cgo */
 StarlarkGo *CgoStarlarkGoAlloc(PyTypeObject *type) {
+  /* Necessary because Cgo can't do function pointers */
   return (StarlarkGo *)type->tp_alloc(type, 0);
 }
 
 void CgoStarlarkGoDealloc(StarlarkGo *self) {
+  /* Necessary because Cgo can't do function pointers */
   Py_TYPE(self)->tp_free((PyObject *)self);
 }
 
-void CgoPyDecRef(PyObject *obj) { Py_XDECREF(obj); }
-
 PyObject *CgoPyBuildOneValue(const char *fmt, const void *src) {
+  /* Necessary because Cgo can't do varargs */
   return Py_BuildValue(fmt, src);
 }
 
-PyObject *CgoPyNone() { Py_RETURN_NONE; }
-
-PyTypeObject *CgoPyType(PyObject *obj) { return Py_TYPE(obj); }
-
-void CgoPyTuple_SET_ITEM(PyObject *tuple, Py_ssize_t pos, PyObject *item) {
-  PyTuple_SET_ITEM(tuple, pos, item);
+PyObject *CgoPyNone() {
+  /* Necessary because Cgo can't do macros */
+  Py_RETURN_NONE;
 }
+
 /* Helper to fetch exception classes */
 static PyObject *get_exception_class(PyObject *errors, const char *name) {
   PyObject *retval = PyObject_GetAttrString(errors, name);

starlark.go

@@ -18,6 +18,7 @@ import (
 	"errors"
 	"math/rand"
 	"reflect"
+	"sync"
 	"unsafe"
 
 	"go.starlark.net/resolve"
@@ -28,6 +29,7 @@ import (
 var (
 	THREADS = map[uint64]*starlark.Thread{}
 	GLOBALS = map[uint64]starlark.StringDict{}
+	MUTEXES = map[uint64]*sync.Mutex{}
 )
 
 func raisePythonException(err error) {
@@ -66,13 +68,13 @@ func raisePythonException(err error) {
 		exc_type = C.EvalError
 	case errors.As(err, &resolveErr):
 		items := C.PyTuple_New(C.Py_ssize_t(len(resolveErr)))
-		defer C.CgoPyDecRef(items)
+		defer C.Py_DecRef(items)
 
 		for i, err := range resolveErr {
 			msg := C.CString(err.Msg)
 			defer C.free(unsafe.Pointer(msg))
 
-			C.CgoPyTuple_SET_ITEM(items, C.Py_ssize_t(i), C.CgoResolveErrorItem(msg, C.uint(err.Pos.Line), C.uint(err.Pos.Col)))
+			C.PyTuple_SetItem(items, C.Py_ssize_t(i), C.CgoResolveErrorItem(msg, C.uint(err.Pos.Line), C.uint(err.Pos.Col)))
 		}
 
 		exc_args = C.CgoResolveErrorArgs(error_msg, error_type, items)
@@ -83,7 +85,7 @@ func raisePythonException(err error) {
 	}
 
 	C.PyErr_SetObject(exc_type, exc_args)
-	C.CgoPyDecRef(exc_args)
+	C.Py_DecRef(exc_args)
 }
 
 //export StarlarkGo_new
@@ -95,8 +97,10 @@ func StarlarkGo_new(pytype *C.PyTypeObject, args *C.PyObject, kwargs *C.PyObject
 
 	threadId := rand.Uint64()
 	thread := &starlark.Thread{}
+
 	THREADS[threadId] = thread
 	GLOBALS[threadId] = starlark.StringDict{}
+	MUTEXES[threadId] = &sync.Mutex{}
 
 	self.starlark_thread = C.ulong(threadId)
 	return self
@@ -105,8 +109,14 @@ func StarlarkGo_new(pytype *C.PyTypeObject, args *C.PyObject, kwargs *C.PyObject
 //export StarlarkGo_dealloc
 func StarlarkGo_dealloc(self *C.StarlarkGo) {
 	threadId := uint64(self.starlark_thread)
+	mutex := MUTEXES[threadId]
+
+	mutex.Lock()
+	defer mutex.Unlock()
+
 	delete(THREADS, threadId)
 	delete(GLOBALS, threadId)
+	delete(MUTEXES, threadId)
 
 	C.CgoStarlarkGoDealloc(self)
 }
@@ -131,10 +141,17 @@ func StarlarkGo_eval(self *C.StarlarkGo, args *C.PyObject, kwargs *C.PyObject) *
 	}
 
 	threadId := uint64(self.starlark_thread)
+	mutex := MUTEXES[threadId]
+
+	mutex.Lock()
+	defer mutex.Unlock()
+
 	thread := THREADS[threadId]
 	globals := GLOBALS[threadId]
-
+	pyThread := C.PyEval_SaveThread()
 	result, err := starlark.Eval(thread, goFilename, goExpr, globals)
+	C.PyEval_RestoreThread(pyThread)
+
 	if err != nil {
 		raisePythonException(err)
 		return nil
@@ -165,9 +182,16 @@ func StarlarkGo_exec(self *C.StarlarkGo, args *C.PyObject, kwargs *C.PyObject) *
 	}
 
 	threadId := uint64(self.starlark_thread)
-	thread := THREADS[threadId]
+	mutex := MUTEXES[threadId]
+
+	mutex.Lock()
+	defer mutex.Unlock()
 
+	thread := THREADS[threadId]
+	pyThread := C.PyEval_SaveThread()
 	globals, err := starlark.ExecFile(thread, goFilename, goDefs, GLOBALS[threadId])
+	C.PyEval_RestoreThread(pyThread)
+
 	if err != nil {
 		raisePythonException(err)
 		return nil

starlark.h

@@ -29,9 +29,6 @@ PyObject *CgoResolveErrorItem(const char *msg, const unsigned int line,
 
 PyObject *CgoResolveErrorArgs(const char *error_msg, const char *error_type,
                               PyObject *errors);
-
-void CgoPyDecRef(PyObject *obj);
-
 PyObject *CgoPyBuildOneValue(const char *fmt, const void *src);
 
 PyObject *CgoPyNone();
@@ -42,8 +39,4 @@ int CgoParseEvalArgs(PyObject *args, PyObject *kwargs, char **expr,
 int GgoParseExecArgs(PyObject *args, PyObject *kwargs, char **defs,
                      char **filename);
 
-PyTypeObject *CgoPyType(PyObject *obj);
-
-void CgoPyTuple_SET_ITEM(PyObject *tuple, Py_ssize_t pos, PyObject *item);
-
 #endif /* PYTHON_STARLARK_GO_H */