Addressing Incorrect Handling of Python GIL that Leads to a SEGFAULT

[Scusemua]

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.

[nickeygit]

Hi @Scusemua !

Thanks for your patch! You saved me several hours of life! :))
And now my turn.

PyGILState locking is needed for sure, but Go is insidious too :)
Goroutines can run on different OS threads
and switch them anytime...

even between PyGILState_Ensure/PyGILState_Release calls!

And sure, PyGILState_Release will panic that current thread has no python thread info and SEGFAULT etc.

My simple two clients-loaded Web server was crashing on this even with your patch.

To fix - we need to lock current goroutine to OS thread during the PyGILState_Ensure/PyGILState_Release section.

Simply add runtime.LockOSThread() call before PyGILState_Ensure (1 place in bind/symbols.go)

and

runtime.UnlockOSThread() after every PyGILState_Release call (3 places in bind/symbols.go)

And now my test server is running fine, calling Python callback in the web handler :)

Feel free to add this to your branch and be brave to propose your PR for merge!

Have nice days guys!

[Scusemua]

Hi @nickeygit,

Yeah, I also found that I needed to pin the goroutines to OS threads, or else I would get really strange errors -- just like you're describing. I just think I forgot to add an update about this to my original post. 😅

With these two fixes, everything has been working fine on my end with no more issues for months! :)

[Scusemua]

I've encountered a new issue in which the application encounters a segmentation fault when calling the resolve callback function.

Sometimes, the segmentation fault occurs on this line:

if C.PyCallable_Check(_fun_arg) == 0 {

Other times, it doesn't occur until C.PyObject_CallObject(_fun_arg, _fcargs).

The issue is with the _fun_arg variable. The _fcargs is fine. I've inspected the callstack in GDB and whatnot, and when the segmentation fault occurs, it's because the _fun_arg is pointing to garbage, rather than the intended PyObject*.

At first, I thought it was perhaps because the callback was being executed before the original thread state was restored by

defer C.PyEval_RestoreThread(_saved_thread)

But I tried adding a sync.WaitGroup such that the callback cannot run until the C.PyEval_RestoreThread(_saved_thread) statement is restored, and the segmentation fault still occurs, with the _fun_arg pointing to garbage...

Has anybody else encountered this? Have you ever experienced this issue, @nickeygit?

[b-long]

@Scusemua I'm sorry I didn't see your work sooner or reach out 😞 I've been getting deeper into gopy and only recently realized how much your changes helped.

I was digging deep into issue 370 and 385 on macos, and after a bunch of failed starts, I realized that your changes made investigation and progress so much more approachable.

I cherry-picked your changes and included them in #393. If you have any thoughts on PR #393, or any questions about it, I'd love to hear them — and I'll do my best to answer.

If you have time, and if you have any input or feedback, I'd be incredibly grateful. Sincerely, thank you for the contributions!

Feel free to close this PR if you're happy with how the work landed in #393.