diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py new file mode 100644 index 00000000..5d9f7cd6 --- /dev/null +++ b/_examples/simple/test_complex_stress.py @@ -0,0 +1,60 @@ +# Copyright 2026 The go-python Authors. All rights reserved. +# Use of this source code is governed by a BSD-style +# license that can be found in the LICENSE file. + +# Regression/stress test for the complex64/complex128 GIL-handling fix. +# +# Comp64Add/Comp128Add marshal a raw *C.PyObject on both sides of the call: +# PyComplex_AsCComplex on the way in, PyComplex_FromDoubles on the way out. +# Both must run while the GIL is held -- gopy releases the GIL only around +# the pure-Go call in between. This hammers the round trip from the main +# thread while a background thread continuously allocates and collects +# Python objects, so that if the marshalling window ever slipped outside the +# GIL-held region, refcount/GC corruption would have a chance to surface. + +import gc +import sys +import threading + +import simple as pkg + +ITERATIONS = 20000 +STOP = threading.Event() + + +def churn(): + while not STOP.is_set(): + _ = [object() for _ in range(100)] + gc.collect(0) + + +t = threading.Thread(target=churn, daemon=True) +t.start() + +try: + for i in range(ITERATIONS): + a = complex(i, -i) + b = complex(-i, i * 2) + want = a + b + + got64 = pkg.Comp64Add(a, b) + # complex64 loses precision relative to Python's complex128 + # arithmetic; compare with a tolerance. + if abs(got64 - want) > 1e-3: + print("FAIL: Comp64Add(%s, %s) = %s, want ~%s" % (a, b, got64, want), + file=sys.stderr) + sys.exit(1) + + got128 = pkg.Comp128Add(a, b) + if got128 != want: + print("FAIL: Comp128Add(%s, %s) = %s, want %s" % (a, b, got128, want), + file=sys.stderr) + sys.exit(1) + + if i % 500 == 0: + gc.collect() +finally: + STOP.set() + t.join() + +print("OK") diff --git a/bind/gen_func.go b/bind/gen_func.go index b2643343..fcc131a2 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -216,6 +216,18 @@ func isIfaceHandle(gdoc string) (bool, string) { return false, gdoc } +// needsGILForArgMarshal reports whether converting a Python-side argument of +// this symbol's type touches a raw *C.PyObject on the Go side (e.g. +// complex64/complex128 via PyComplex_AsCComplex). Such conversions must run +// while the GIL is still held, not inline in the wrapped call after +// C.PyEval_SaveThread has released it -- touching a PyObject without the GIL +// is undefined behavior. The py2go signature closure (isSignature) is +// excluded: it is invoked later, from inside the wrapped Go call, and already +// reacquires the GIL itself via PyGILState_Ensure/Release around its body. +func needsGILForArgMarshal(sym *symbol) bool { + return sym.cgoname == "*C.PyObject" && !sym.isSignature() && sym.py2go != "" +} + func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { isMethod := (sym != nil) isIface := false @@ -261,6 +273,24 @@ func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { } } + // Convert any argument whose marshalling touches a raw *C.PyObject while + // the GIL is still held -- before it is released below for the duration + // of the wrapped Go call. The variadic tail is skipped: its "arg" stands + // in for the whole trailing slice, not a single marshalled scalar. + premarshalled := make(map[int]string) + for i, arg := range args { + if !needsGILForArgMarshal(arg.sym) { + continue + } + if fsym.isVariadic && i == len(args)-1 { + continue + } + anm := pySafeArg(arg.Name(), i) + varnm := fmt.Sprintf("_premarshal%d", i) + g.gofile.Printf("%s := %s(%s)%s\n", varnm, arg.sym.py2go, anm, arg.sym.py2goParenEx) + premarshalled[i] = varnm + } + g.gofile.Printf("_saved_thread := C.PyEval_SaveThread()\n") if !rvIsErr && nres != 2 { g.gofile.Printf("defer C.PyEval_RestoreThread(_saved_thread)\n") @@ -304,6 +334,8 @@ if __err != nil { na = fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm) case arg.sym.isSignature(): na = fmt.Sprintf("%s", arg.sym.py2go) + case premarshalled[i] != "": + na = premarshalled[i] case arg.sym.py2go != "": na = fmt.Sprintf("%s(%s)%s", arg.sym.py2go, anm, arg.sym.py2goParenEx) default: diff --git a/bind/symbols.go b/bind/symbols.go index 67bb6a5b..d560ac8e 100644 --- a/bind/symbols.go +++ b/bind/symbols.go @@ -636,8 +636,19 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) // bstr += fmt.Sprintf("}\n") // } + // PyTuple_New/PyTuple_SetItem below touch raw *C.PyObject values and + // must run with the GIL held. This code is spliced verbatim into a + // caller-generated function body, so it cannot assume the GIL is + // already held there -- bracket it here instead of relying on every + // caller to do so correctly (PyGILState_Ensure/Release nest safely, so + // this is sound even when the caller already holds the GIL itself). + // varnm is declared outside the block so it stays visible to + // caller-emitted code that follows (e.g. PyObject_CallObject). + // // TODO: more efficient to use strings.Builder here.. - bstr := fmt.Sprintf("%s := C.PyTuple_New(%d)\n", varnm, sz) + bstr := fmt.Sprintf("var %s *C.PyObject\n{\n", varnm) + bstr += "_tuple_gstate := C.PyGILState_Ensure()\n" + bstr += fmt.Sprintf("%s = C.PyTuple_New(%d)\n", varnm, sz) for i := 0; i < sz; i++ { v := tuple.At(i) typ := v.Type() @@ -679,6 +690,8 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) return "", fmt.Errorf("buildTuple: type not handled: %s", typ.String()) } } + bstr += "C.PyGILState_Release(_tuple_gstate)\n" + bstr += "}\n" return bstr, nil } diff --git a/bind/symbols_test.go b/bind/symbols_test.go new file mode 100644 index 00000000..68449c41 --- /dev/null +++ b/bind/symbols_test.go @@ -0,0 +1,60 @@ +// Copyright 2026 The go-python Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package bind + +import ( + "go/token" + "go/types" + "strings" + "testing" +) + +// TestBuildTupleHoldsGIL guards against a regression class already fixed +// once for complex64/complex128 (see needsGILForArgMarshal in gen_func.go): +// generated code that touches a raw *C.PyObject must never run outside a +// PyGILState_Ensure/Release (or equivalent) bracket. +// +// buildTuple emits PyTuple_New/PyTuple_SetItem -- pure PyObject manipulation +// -- as a bare code fragment with no way to know whether its caller already +// holds the GIL. Its one current caller (addSignatureType's py2go closure) +// happens to bracket it correctly today, but that's a convention external +// to buildTuple, not something buildTuple enforces. This test requires +// buildTuple's own output to be self-bracketing, so a future caller can't +// reintroduce the complex64/128 bug by forgetting to hold the GIL. +func TestBuildTupleHoldsGIL(t *testing.T) { + tuple := types.NewTuple( + types.NewVar(token.NoPos, nil, "i", types.Typ[types.Int]), + types.NewVar(token.NoPos, nil, "s", types.Typ[types.String]), + ) + + got, err := current.buildTuple(tuple, "_fcargs", "_fun_arg") + if err != nil { + t.Fatalf("buildTuple returned error: %v", err) + } + + ensureCount := strings.Count(got, "C.PyGILState_Ensure()") + releaseCount := strings.Count(got, "C.PyGILState_Release(") + if ensureCount == 0 || releaseCount == 0 { + t.Fatalf("buildTuple output does not self-bracket its PyTuple_* calls "+ + "with PyGILState_Ensure/Release; got:\n%s", got) + } + if ensureCount != releaseCount { + t.Fatalf("mismatched PyGILState_Ensure/Release counts (%d vs %d) in "+ + "buildTuple output:\n%s", ensureCount, releaseCount, got) + } + + ensureIdx := strings.Index(got, "C.PyGILState_Ensure()") + firstTupleIdx := strings.Index(got, "C.PyTuple_") + lastTupleIdx := strings.LastIndex(got, "C.PyTuple_") + releaseIdx := strings.LastIndex(got, "C.PyGILState_Release(") + + if ensureIdx == -1 || firstTupleIdx == -1 || releaseIdx == -1 { + t.Fatalf("could not locate expected markers in buildTuple output:\n%s", got) + } + if !(ensureIdx < firstTupleIdx && lastTupleIdx < releaseIdx) { + t.Fatalf("PyTuple_* calls are not nested inside the "+ + "PyGILState_Ensure/Release bracket in buildTuple output:\n%s", got) + } +} diff --git a/main_test.go b/main_test.go index 1127698c..e150c485 100644 --- a/main_test.go +++ b/main_test.go @@ -851,6 +851,64 @@ func TestGilString(t *testing.T) { } } +// TestComplexGILStress is a regression/stress test for the complex64/128 +// GIL-handling fix. Comp64Add/Comp128Add marshal a raw *C.PyObject on both +// sides of the call (PyComplex_AsCComplex on the argument-read side, +// PyComplex_FromDoubles on the return side); both must run while the GIL is +// held, since gopy releases it only around the pure-Go call in between. This +// repeatedly exercises the round trip under GC/allocation pressure from a +// background thread so a mis-timed marshalling window has a chance to +// surface as corruption rather than passing silently. +func TestComplexGILStress(t *testing.T) { + backends := []string{"py3"} + for _, be := range backends { + vm, ok := testBackends[be] + if !ok || vm == "" { + t.Logf("Skipped testing backend %s for TestComplexGILStress\n", be) + continue + } + t.Run(be, func(t *testing.T) { + cwd, _ := os.Getwd() + + workdir, err := os.MkdirTemp("", "gopy-") + if err != nil { + t.Fatalf("could not create workdir: %v", err) + } + defer os.RemoveAll(workdir) + defer bind.ResetPackages() + + writeGoMod(t, cwd, workdir) + if err := run([]string{"build", "-vm=" + vm, "-output=" + workdir, "-package-prefix", "", "./_examples/simple"}); err != nil { + t.Fatalf("error building simple: %v", err) + } + + tstDst := filepath.Join(workdir, "test_complex_stress.py") + if err := copyCmd(filepath.Join(cwd, "_examples/simple/test_complex_stress.py"), tstDst); err != nil { + t.Fatalf("error copying test_complex_stress.py: %v", err) + } + + env := make([]string, len(testEnvironment)) + copy(env, testEnvironment) + env = append(env, fmt.Sprintf("PYTHONPATH=%s", workdir)) + + cmd := exec.Command(vm, "./test_complex_stress.py") + cmd.Env = env + cmd.Dir = workdir + cmd.Stdin = os.Stdin + buf, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("error running python module: err=%v\n%s", err, string(buf)) + } + + got := strings.Replace(string(buf), "\r\n", "\n", -1) + want := "OK\n" + if got != want { + t.Fatalf("got:\n%s\nwant:\n%s", got, want) + } + }) + } +} + func TestPackagePrefix(t *testing.T) { // t.Parallel() path := "_examples/package/mypkg"