Addressing Incorrect Handling of Python GIL that Leads to a SEGFAULT

I found what appears to be a bug in bind/symbols.go.

Consider the following Go function that was generated by gopy:

func gopackage_GoObject_ExampleFunc(_handle CGoHandle, val CGoHandle, pythonCallback *C.PyObject, callbackArgument *C.char, goRun C.char) {
    _fun_arg := pythonCallback
    _saved_thread := C.PyEval_SaveThread() // Release GIL
    defer C.PyEval_RestoreThread(_saved_thread) // Acquire GIL (deferred) 
    vifc, __err := gopyh.VarFromHandleTry((gopyh.CGoHandle)(_handle), "*gopackage.GoObject")
    if __err != nil {
        return
    }
    if boolPyToGo(goRun) {
        go gopyh.Embed(vifc, reflect.TypeOf(gopackage.GoObject{})).(*gopackage.GoObject).ExampleFunc(*ptrFromHandle_gopackage_Bytes(val), func(arg_0 interface{}, arg_1 string) {
            if C.PyCallable_Check(_fun_arg) == 0 {
                return
            }
            _gstate := C.PyGILState_Ensure() // Acquire GIL
            _fcargs := C.PyTuple_New(2)
            C.PyTuple_SetItem(_fcargs, 0, C.gopy_build_string(C.CString(fmt.Sprintf("%s", (arg_0)))))
            C.PyTuple_SetItem(_fcargs, 1, C.gopy_build_string(C.CString(arg_1)))
            C.PyObject_CallObject(_fun_arg, _fcargs)
            C.gopy_decref(_fcargs)
            C.gopy_err_handle()
            C.PyGILState_Release(_gstate) // Release GIL
        }, C.GoString(callbackArgument))
    } else {
        gopyh.Embed(vifc, reflect.TypeOf(gopackage.GoObject{})).(*gopackage.GoObject).ExampleFunc(*ptrFromHandle_gopackage_Bytes(val), func(arg_0 interface{}, arg_1 string) {
            if C.PyCallable_Check(_fun_arg) == 0 {
                return
            }
            _gstate := C.PyGILState_Ensure() // Acquire GIL
            _fcargs := C.PyTuple_New(2)
            C.PyTuple_SetItem(_fcargs, 0, C.gopy_build_string(C.CString(fmt.Sprintf("%s", (arg_0)))))
            C.PyTuple_SetItem(_fcargs, 1, C.gopy_build_string(C.CString(arg_1)))
            C.PyObject_CallObject(_fun_arg, _fcargs)
            C.gopy_decref(_fcargs)
            C.gopy_err_handle()
            C.PyGILState_Release(_gstate) // Release GIL
        }, C.GoString(callbackArgument))
    }
}

This is generated for a Go method ExampleFunction of a GoObject struct. The intent is for this method to be called from Python.

The bug is that we access the C/Python API function C.PyCallable_Check before acquiring the GIL, leading to a SEGFAULT. The problem lies in the following excerpt of the above function:

            if C.PyCallable_Check(_fun_arg) == 0 {
                return
            }
            _gstate := C.PyGILState_Ensure() // Acquire GIL
            _fcargs := C.PyTuple_New(2)

Notice that we only acquire the GIL via PyGILState_Ensure after the call to C.PyCallable_Check(_fun_arg), which leads to a SEGFAULT.

The changes in this PR will result in the above Go function being generated differently as:

func gopackage_GoObject_ExampleFunc(_handle CGoHandle, val CGoHandle, pythonCallback *C.PyObject, callbackArgument *C.char, goRun C.char) {
    _fun_arg := pythonCallback
    _saved_thread := C.PyEval_SaveThread()  // Release GIL
    defer C.PyEval_RestoreThread(_saved_thread) // Acquire GIL (deferred) 
    vifc, __err := gopyh.VarFromHandleTry((gopyh.CGoHandle)(_handle), "*gopackage.GoObject")
    if __err != nil {
        return
    }
    if boolPyToGo(goRun) {
        go gopyh.Embed(vifc, reflect.TypeOf(gopackage.GoObject{})).(*gopackage.GoObject).ExampleFunc(*ptrFromHandle_gopackage_Bytes(val), func(arg_0 interface{}, arg_1 string) {
            _gstate := C.PyGILState_Ensure() // Acquire GIL
            if C.PyCallable_Check(_fun_arg) == 0 {
                C.PyGILState_Release(_gstate) // Release GIL
                return
            }
            _fcargs := C.PyTuple_New(2)
            C.PyTuple_SetItem(_fcargs, 0, C.gopy_build_string(C.CString(fmt.Sprintf("%s", (arg_0)))))
            C.PyTuple_SetItem(_fcargs, 1, C.gopy_build_string(C.CString(arg_1)))
            C.PyObject_CallObject(_fun_arg, _fcargs)
            C.gopy_decref(_fcargs)
            C.gopy_err_handle()
            C.PyGILState_Release(_gstate) // Release GIL
        }, C.GoString(callbackArgument))
    } else {
        gopyh.Embed(vifc, reflect.TypeOf(gopackage.GoObject{})).(*gopackage.GoObject).ExampleFunc(*ptrFromHandle_gopackage_Bytes(val), func(arg_0 interface{}, arg_1 string) {
            _gstate := C.PyGILState_Ensure() // Acquire GIL
            if C.PyCallable_Check(_fun_arg) == 0 {
                C.PyGILState_Release(_gstate) // Release GIL
                return
            }
            _fcargs := C.PyTuple_New(2)
            C.PyTuple_SetItem(_fcargs, 0, C.gopy_build_string(C.CString(fmt.Sprintf("%s", (arg_0)))))
            C.PyTuple_SetItem(_fcargs, 1, C.gopy_build_string(C.CString(arg_1)))
            C.PyObject_CallObject(_fun_arg, _fcargs)
            C.gopy_decref(_fcargs)
            C.gopy_err_handle()
            C.PyGILState_Release(_gstate) // Release GIL
        }, C.GoString(callbackArgument))
    }
}

Notice that we now call C.PyGILState_Ensure() before the call to C.PyCallable_Check(_fun_arg). Likewise, we call C.PyGILState_Release(_gstate) before returning within the body of the associated if-statement to ensure that there is a matching call (to match the _gstate := C.PyGILState_Ensure()).

In terms of testing, I've just been using the modified gopy in an application I'm developing. I'm in the process of doing more testing in the meantime; however, I am confident that this is a bug, as you must hold the Python GIL when interfacing/invoking any of the C/Python API.

From the "Thread State and the Global Interpreter Lock" subsection of the "Initialization, Finalization, and Threads" Python C-API documentation:

"Therefore, the rule exists that only the thread that has acquired the GIL may operate on Python objects or call Python/C API functions."

On a separate (but related) note -- since we defer C.PyEval_RestoreThread(_saved_thread) at the beginning of the method's execution, it's a little silly to release the GIL via C.PyGILState_Release(_gstate) immediately before returning (even in the case where we don't call into Go code); however, we need to have matching calls between C.PyEval_SaveThread() and C.PyEval_RestoreThread() as well as PyGILState_Ensure() and PyGILState_Release(), so I think it ultimately makes sense to do things this way for now. In theory, we could provide more complex logic to handle the GIL more efficiently in the future.

go-python / gopy

Addressing Incorrect Handling of Python GIL that Leads to a SEGFAULT #361