From ec3b50ca9c7e5c244624e7acfce5b50ec4894cc1 Mon Sep 17 00:00:00 2001 From: b-long Date: Fri, 31 Jul 2026 15:43:02 -0400 Subject: [PATCH 1/8] bind: hold GIL while marshalling complex arguments and building tuples Convert Python-side arguments whose marshalling touches a raw *C.PyObject (complex64/complex128, via needsGILForArgMarshal) before releasing the GIL for the wrapped Go call, instead of inline afterward once it has already been released. Self-bracket buildTuple's PyTuple_New/PyTuple_SetItem output with PyGILState_Ensure/Release so it does not rely on its caller already holding the GIL. Adds a complex64/128 round-trip stress test that hammers the fix from a churn thread, and TestBuildTupleHoldsGIL. --- _examples/simple/test_complex_stress.py | 60 +++++++++++++++++++++++++ bind/gen_func.go | 32 +++++++++++++ bind/symbols.go | 15 ++++++- bind/symbols_test.go | 60 +++++++++++++++++++++++++ main_test.go | 58 ++++++++++++++++++++++++ 5 files changed, 224 insertions(+), 1 deletion(-) create mode 100644 _examples/simple/test_complex_stress.py create mode 100644 bind/symbols_test.go 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" From e24deba8bfc45af4b8d751374cb7bea0549280fc Mon Sep 17 00:00:00 2001 From: b-long Date: Fri, 31 Jul 2026 15:44:09 -0400 Subject: [PATCH 2/8] bind: fix GIL release ordering and leak in signature callbacks addSignatureType released the GIL before converting the *C.PyObject return value from an invoked Python callback back to Go (e.g. via PyLong_AsLongLong/PyBytes_AsString) and never decref'd it. Both now happen before PyGILState_Release, closing a live GIL-less window (hit by any callback with a return value, e.g. _examples/funcs' CallBackRval) and a one-object-per-call leak. Drop buildTuple's self-bracketing GIL, added in the previous commit: its only caller (addSignatureType) already holds the GIL across the whole build/call/decref region, so the extra Ensure/Release was redundant. Replace TestBuildTupleHoldsGIL, which only checked the tuple-build fragment and could pass in the broken case, with a test on addSignatureType's full output. Extract argCallExpr as the single function deciding both an argument's call-site expression and whether it needs GIL premarshalling, so a premarshalled variable can no longer be declared without also being the expression consumed at the call site. Trim the complex stress test from 20000 to 500 iterations; the pre-fix crash is deterministic on the first call. --- _examples/simple/test_complex_stress.py | 4 +- bind/gen_func.go | 64 +++++++++----- bind/gen_func_test.go | 111 ++++++++++++++++++++++++ bind/symbols.go | 36 +++++--- bind/symbols_test.go | 87 +++++++++++-------- 5 files changed, 231 insertions(+), 71 deletions(-) create mode 100644 bind/gen_func_test.go diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py index 5d9f7cd6..80beadf3 100644 --- a/_examples/simple/test_complex_stress.py +++ b/_examples/simple/test_complex_stress.py @@ -18,7 +18,7 @@ import simple as pkg -ITERATIONS = 20000 +ITERATIONS = 500 STOP = threading.Event() @@ -51,7 +51,7 @@ def churn(): file=sys.stderr) sys.exit(1) - if i % 500 == 0: + if i % 50 == 0: gc.collect() finally: STOP.set() diff --git a/bind/gen_func.go b/bind/gen_func.go index fcc131a2..5ec5067f 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -228,6 +228,37 @@ func needsGILForArgMarshal(sym *symbol) bool { return sym.cgoname == "*C.PyObject" && !sym.isSignature() && sym.py2go != "" } +// argCallExpr returns the Go-side expression used to pass a Python-wrapped +// argument to the wrapped Go call, and whether that expression must be +// evaluated before the GIL is released (see needsGILForArgMarshal). anm is +// the argument's Python-side (pySafeArg) name; isVariadicTail marks the +// trailing "arg" that stands in for a whole variadic slice rather than a +// single marshalled scalar. +// +// This is the single place that decides both questions, in one precedence +// order: ifchandle/interface{} and isSignature never touch a raw PyObject +// via this expression (isSignature's closure re-acquires the GIL itself), +// so they always report needsGIL=false, regardless of the symbol's cgoname. +// Callers must call this once per argument for the premarshal decision and +// again (with identical inputs) for the call-argument expression -- because +// both calls run the same deterministic logic, a "_premarshalN" can never +// be declared under one precedence and then left unreferenced under +// another, the way it could when the two decisions were made by separately +// maintained code. +func argCallExpr(ifchandle, isVariadicTail bool, anm string, sym *symbol) (expr string, needsGIL bool) { + switch { + case ifchandle && sym.goname == "interface{}": + return fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm), false + case sym.isSignature(): + return sym.py2go, false + case sym.py2go != "": + expr := fmt.Sprintf("%s(%s)%s", sym.py2go, anm, sym.py2goParenEx) + return expr, needsGILForArgMarshal(sym) && !isVariadicTail + default: + return anm, false + } +} + func (g *pyGen) genFuncBody(sym *symbol, fsym *Func) { isMethod := (sym != nil) isIface := false @@ -275,19 +306,20 @@ 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. + // of the wrapped Go call. argCallExpr is also what decides each + // argument's call-site expression below, so a premarshalled variable is + // declared here if and only if it is also the expression consumed + // there. premarshalled := make(map[int]string) for i, arg := range args { - if !needsGILForArgMarshal(arg.sym) { - continue - } - if fsym.isVariadic && i == len(args)-1 { + anm := pySafeArg(arg.Name(), i) + isVariadicTail := fsym.isVariadic && i == len(args)-1 + expr, needsGIL := argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) + if !needsGIL { 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) + g.gofile.Printf("%s := %s\n", varnm, expr) premarshalled[i] = varnm } @@ -327,21 +359,13 @@ if __err != nil { wrapArgs = append(wrapArgs, "self.handle") } for i, arg := range args { - na := "" anm := pySafeArg(arg.Name(), i) - switch { - case ifchandle && arg.sym.goname == "interface{}": - na = fmt.Sprintf(`gopyh.VarFromHandle((gopyh.CGoHandle)(%s), "interface{}")`, anm) - case arg.sym.isSignature(): - na = fmt.Sprintf("%s", arg.sym.py2go) - case premarshalled[i] != "": + isVariadicTail := fsym.isVariadic && i == len(args)-1 + na, needsGIL := argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) + if needsGIL { na = premarshalled[i] - case arg.sym.py2go != "": - na = fmt.Sprintf("%s(%s)%s", arg.sym.py2go, anm, arg.sym.py2goParenEx) - default: - na = anm } - if i == len(args)-1 && fsym.isVariadic { + if isVariadicTail { na = na + "..." } callArgs = append(callArgs, na) diff --git a/bind/gen_func_test.go b/bind/gen_func_test.go new file mode 100644 index 00000000..e8fda48d --- /dev/null +++ b/bind/gen_func_test.go @@ -0,0 +1,111 @@ +// 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 "testing" + +// TestArgCallExprPrecedenceMatchesPremarshalDecision guards against the +// failure mode flagged in the PR #401 review of needsGILForArgMarshal: the +// call-argument expression for a Python-wrapped argument used to be decided +// by a switch with a fixed case order (ifchandle&&interface{} > isSignature +// > premarshalled > py2go > default), while a *separate* loop decided, +// independently, whether to emit a "_premarshalN := ..." declaration before +// the GIL is released. Nothing today makes an argument match both an +// earlier switch case and needsGILForArgMarshal, but if it ever did, the +// premarshal declaration would be emitted and never referenced -- +// "_premarshalN declared and not used" -- a compile error surfacing in the +// user's generated package, not in gopy. +// +// argCallExpr is the fix: both the premarshal pass and the call-argument +// pass now call this single function with the same inputs, so a +// premarshalled variable is declared if and only if it is also the +// expression that gets used. This test exercises that precedence directly, +// including a contrived interface{}/*C.PyObject collision that cannot +// happen with today's type table (interface{}'s cgoname is "*C.char") but +// is exactly the shape the review warned about. +func TestArgCallExprPrecedenceMatchesPremarshalDecision(t *testing.T) { + complex128Sym := &symbol{ + goname: "complex128", + cgoname: "*C.PyObject", + py2go: "complex128FromPyObject", + py2goParenEx: "", + } + + ifaceHandleSym := &symbol{ + goname: "interface{}", + cgoname: "*C.PyObject", // contrived: not true of any symbol today + py2go: "wouldBeMarshalFunc", + py2goParenEx: "", + } + + sigSym := &symbol{ + kind: skSignature, + goname: "func(int) string", + cgoname: "*C.PyObject", + py2go: "func (x int) string { ... }", + py2goParenEx: "", + } + + plainStringSym := &symbol{ + goname: "string", + cgoname: "*C.char", + py2go: "C.GoString", + py2goParenEx: "", + } + + tests := []struct { + name string + ifchandle bool + isVariadic bool + sym *symbol + wantExpr string + wantNeedsGIL bool + }{ + { + name: "complex128 arg is premarshalled", + sym: complex128Sym, + wantExpr: "complex128FromPyObject(x)", + wantNeedsGIL: true, + }, + { + name: "complex128 arg in variadic tail is not premarshalled", + isVariadic: true, + sym: complex128Sym, + wantExpr: "complex128FromPyObject(x)", + wantNeedsGIL: false, + }, + { + name: "ifchandle interface{} wins over premarshalling even if cgoname collides", + ifchandle: true, + sym: ifaceHandleSym, + wantExpr: `gopyh.VarFromHandle((gopyh.CGoHandle)(x), "interface{}")`, + wantNeedsGIL: false, + }, + { + name: "isSignature wins over premarshalling", + sym: sigSym, + wantExpr: "func (x int) string { ... }", + wantNeedsGIL: false, + }, + { + name: "plain string arg is not premarshalled", + sym: plainStringSym, + wantExpr: "C.GoString(x)", + wantNeedsGIL: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotExpr, gotNeedsGIL := argCallExpr(tt.ifchandle, tt.isVariadic, "x", tt.sym) + if gotExpr != tt.wantExpr { + t.Errorf("argCallExpr() expr = %q, want %q", gotExpr, tt.wantExpr) + } + if gotNeedsGIL != tt.wantNeedsGIL { + t.Errorf("argCallExpr() needsGIL = %v, want %v", gotNeedsGIL, tt.wantNeedsGIL) + } + }) + } +} diff --git a/bind/symbols.go b/bind/symbols.go index d560ac8e..3d0fea38 100644 --- a/bind/symbols.go +++ b/bind/symbols.go @@ -638,17 +638,19 @@ func (sym *symtab) buildTuple(tuple *types.Tuple, varnm string, methvar string) // 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). + // caller-generated function body, so the caller is responsible for + // holding the GIL across it -- addSignatureType (its only caller) does + // so already, bracketing this output together with the + // PyObject_CallObject/decref/return-conversion that follows it in the + // same generated closure, under one PyGILState_Ensure/Release. Do not + // re-bracket it here: a self-contained Ensure/Release around just this + // fragment would be redundant with the caller's (nesting is safe, so it + // wouldn't break anything), but it also wouldn't buy anything, since + // the built tuple is only ever used later, by the same caller, which + // must keep holding the GIL regardless. // // TODO: more efficient to use strings.Builder here.. - 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) + bstr := fmt.Sprintf("%s := C.PyTuple_New(%d)\n", varnm, sz) for i := 0; i < sz; i++ { v := tuple.At(i) typ := v.Type() @@ -690,8 +692,6 @@ 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 } @@ -1128,13 +1128,23 @@ func (sym *symtab) addSignatureType(pkg *types.Package, obj types.Object, t type py2g += retstr + "C.PyObject_CallObject(_fun_arg, nil)\n" } py2g += "C.gopy_err_handle()\n" - py2g += "C.PyGILState_Release(_gstate)\n" + // _fcret (if any) is a *C.PyObject -- a new reference returned by + // PyObject_CallObject. Converting it to a Go value (pyObjectToGo, e.g. + // PyLong_AsLongLong/PyBytes_AsString) and decref'ing it both touch a + // raw PyObject and so must happen before PyGILState_Release, not after. + // The converted value is stashed in a local so it can still be returned + // once the GIL is no longer held. if rets.Len() == 1 { cvt, err := sym.pyObjectToGo(ret.Type(), rsym, "_fcret") if err != nil { return err } - py2g += fmt.Sprintf("return %s", cvt) + py2g += fmt.Sprintf("_fcretgo := %s\n", cvt) + py2g += "C.gopy_decref(_fcret)\n" + } + py2g += "C.PyGILState_Release(_gstate)\n" + if rets.Len() == 1 { + py2g += "return _fcretgo" } py2g += "}" diff --git a/bind/symbols_test.go b/bind/symbols_test.go index 68449c41..4bb8e00a 100644 --- a/bind/symbols_test.go +++ b/bind/symbols_test.go @@ -11,50 +11,65 @@ import ( "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. +// TestAddSignatureTypeHoldsGILThroughoutClosure guards against the family of +// GIL bugs already fixed once for complex64/complex128 arguments (see +// needsGILForArgMarshal in gen_func.go): generated code that touches a raw +// *C.PyObject must never run outside a PyGILState_Ensure/Release 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) - } +// addSignatureType's py2go closure is the one place this whole family shows +// up together: it builds a PyTuple from the Go-side arguments (PyTuple_New/ +// SetItem, via buildTuple), invokes the Python callback +// (PyObject_CallObject), and converts the *C.PyObject result back to a Go +// value (pyObjectToGo, e.g. PyBytes_AsString) before decref'ing it. All of +// that touches raw PyObjects and must happen inside the single +// PyGILState_Ensure/Release the closure takes out -- not just the tuple +// build (buildTuple itself does not bracket its own output; it relies on +// its only caller, this closure, to hold the GIL across build, call, and +// return-conversion together). Getting the release point wrong here is not +// hypothetical: _examples/funcs' CallBackRval passes a bool-returning +// callback through exactly this path today. +func TestAddSignatureTypeHoldsGILThroughoutClosure(t *testing.T) { + sig := types.NewSignature(nil, + types.NewTuple(types.NewVar(token.NoPos, nil, "x", types.Typ[types.Int])), + types.NewTuple(types.NewVar(token.NoPos, nil, "", types.Typ[types.String])), + false) - 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 err := current.addSignatureType(nil, nil, sig, 0, "sigtest_id", "sigtest"); err != nil { + t.Fatalf("addSignatureType returned error: %v", err) } - if ensureCount != releaseCount { - t.Fatalf("mismatched PyGILState_Ensure/Release counts (%d vs %d) in "+ - "buildTuple output:\n%s", ensureCount, releaseCount, got) + + sym := current.symtype(sig) + if sym == nil { + t.Fatalf("addSignatureType did not register a symbol for %s", current.fullTypeString(sig)) } + got := sym.py2go 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(") + callIdx := strings.Index(got, "C.PyObject_CallObject(") + convIdx := strings.Index(got, "C.PyBytes_AsString(_fcret)") + decrefIdx := strings.Index(got, "C.gopy_decref(_fcret)") + releaseIdx := strings.LastIndex(got, "C.PyGILState_Release(_gstate)") - if ensureIdx == -1 || firstTupleIdx == -1 || releaseIdx == -1 { - t.Fatalf("could not locate expected markers in buildTuple output:\n%s", got) + for name, idx := range map[string]int{ + "PyGILState_Ensure": ensureIdx, + "PyTuple_*": lastTupleIdx, + "PyObject_CallObject": callIdx, + "PyBytes_AsString": convIdx, + "gopy_decref(_fcret)": decrefIdx, + "final PyGILState_Release": releaseIdx, + } { + if idx == -1 { + t.Fatalf("could not locate expected %s call in generated closure:\n%s", name, 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) + + // Everything that touches a PyObject -- building the tuple, invoking + // the callback, converting and decref'ing the result -- must occur + // strictly between the Ensure and the final Release. + if !(ensureIdx < lastTupleIdx && lastTupleIdx < callIdx && callIdx < convIdx && + convIdx < releaseIdx && decrefIdx < releaseIdx) { + t.Fatalf("generated closure does not hold the GIL across the entire "+ + "build-tuple/call/convert/decref sequence:\n%s", got) } } From a6e4860bf8988089191a858ce2e7332d68e2dd7a Mon Sep 17 00:00:00 2001 From: b-long Date: Sat, 7 Nov 2026 14:18:25 -0500 Subject: [PATCH 3/8] bind: compute each argument's call expression once in genFuncBody genFuncBody called argCallExpr twice per argument, once in the premarshal pass and again at the call site, and relied on both calls agreeing. Since the merge with master, argCallExpr's callback case goes through shimABI.callbackArg, which for the plain-C backends also registers the callback's trampoline (g.trampoline). That registration is deduplicated, so a second call was harmless, but correctness should not depend on it. The premarshal pass now records each argument's expression (or the _premarshalN variable that holds it) in argExprs, and the call site uses that directly. A premarshalled variable is declared exactly when it is the expression passed to the call, by construction rather than because two calls happen to agree. --- bind/gen_func.go | 39 +++++++++++++++++---------------------- bind/gen_func_test.go | 8 ++++---- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/bind/gen_func.go b/bind/gen_func.go index 9c704243..0814d93d 100644 --- a/bind/gen_func.go +++ b/bind/gen_func.go @@ -297,12 +297,10 @@ func needsGILForArgMarshal(sym *symbol) bool { // order: ifchandle/interface{} and isSignature never touch a raw PyObject // via this expression (a callback's wrapper re-acquires the GIL itself, see // shimABI.callbackArg), so they always report needsGIL=false, regardless of -// the symbol's cgoname. Callers must call this once per argument for the -// premarshal decision and again (with identical inputs) for the -// call-argument expression -- because both calls run the same deterministic -// logic, a "_premarshalN" can never be declared under one precedence and -// then left unreferenced under another, the way it could when the two -// decisions were made by separately maintained code. +// the symbol's cgoname. genFuncBody calls it exactly once per argument and +// uses the returned expression (or the premarshalled variable holding it) +// at the call site, so a "_premarshalN" can never be declared under one +// precedence and then left unreferenced under another. func (g *pyGen) argCallExpr(ifchandle, isVariadicTail bool, anm string, sym *symbol) (expr string, needsGIL bool) { switch { case ifchandle && sym.goname == "interface{}": @@ -362,23 +360,23 @@ 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. argCallExpr is also what decides each - // argument's call-site expression below, so a premarshalled variable is - // declared here if and only if it is also the expression consumed - // there. - premarshalled := make(map[int]string) + // Decide each argument's call-site expression once, up front, and convert + // any argument whose marshalling touches a raw *C.PyObject now, while the + // GIL is still held -- before saveThread releases it below for the + // duration of the wrapped Go call. The call-argument loop further down + // consumes argExprs as-is, so a premarshalled variable is declared here if + // and only if it is also the expression used at the call site. + argExprs := make([]string, len(args)) for i, arg := range args { anm := pySafeArg(arg.Name(), i) isVariadicTail := fsym.isVariadic && i == len(args)-1 expr, needsGIL := g.argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) - if !needsGIL { - continue + if needsGIL { + varnm := fmt.Sprintf("_premarshal%d", i) + g.gofile.Printf("%s := %s\n", varnm, expr) + expr = varnm } - varnm := fmt.Sprintf("_premarshal%d", i) - g.gofile.Printf("%s := %s\n", varnm, expr) - premarshalled[i] = varnm + argExprs[i] = expr } g.gofile.Print(g.shim.saveThread(!rvIsErr && nres != 2)) @@ -416,10 +414,7 @@ if __err != nil { for i, arg := range args { anm := pySafeArg(arg.Name(), i) isVariadicTail := fsym.isVariadic && i == len(args)-1 - na, needsGIL := g.argCallExpr(ifchandle, isVariadicTail, anm, arg.sym) - if needsGIL { - na = premarshalled[i] - } + na := argExprs[i] if isVariadicTail { na = na + "..." } diff --git a/bind/gen_func_test.go b/bind/gen_func_test.go index 1c2bd9c2..e9424c0b 100644 --- a/bind/gen_func_test.go +++ b/bind/gen_func_test.go @@ -18,10 +18,10 @@ import "testing" // "_premarshalN declared and not used" -- a compile error surfacing in the // user's generated package, not in gopy. // -// argCallExpr is the fix: both the premarshal pass and the call-argument -// pass now call this single function with the same inputs, so a -// premarshalled variable is declared if and only if it is also the -// expression that gets used. This test exercises that precedence directly, +// argCallExpr is the fix: genFuncBody calls this single function once per +// argument and uses its result both for the premarshal decision and at the +// call site, so a premarshalled variable is declared if and only if it is +// also the expression that gets used. This test exercises that precedence directly, // including a contrived interface{}/*C.PyObject collision that cannot // happen with today's type table (interface{}'s cgoname is "*C.char") but // is exactly the shape the review warned about. From 60b0febc64919501b2ed0ae70a0db823cfbdb3ba Mon Sep 17 00:00:00 2001 From: b-long Date: Sat, 7 Nov 2026 14:22:57 -0500 Subject: [PATCH 4/8] test: run TestComplexGILStress's gopy as a subprocess, in parallel Master's main_test.go now builds the gopy binary once in TestMain and has tests run it through runGopy, so they can call t.Parallel() (#416). Running gopy in-process is unsafe there, because the generator keeps its state in package variables. Move TestComplexGILStress to runGopy and t.Parallel(), like TestGilString, and drop its bind.ResetPackages(), which only cleaned up the in-process run. --- main_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/main_test.go b/main_test.go index 35b705eb..8a8f9465 100644 --- a/main_test.go +++ b/main_test.go @@ -1241,6 +1241,7 @@ func TestGilString(t *testing.T) { // background thread so a mis-timed marshalling window has a chance to // surface as corruption rather than passing silently. func TestComplexGILStress(t *testing.T) { + t.Parallel() backends := []string{"py3"} for _, be := range backends { vm, ok := testBackends[be] @@ -1249,6 +1250,7 @@ func TestComplexGILStress(t *testing.T) { continue } t.Run(be, func(t *testing.T) { + t.Parallel() cwd, _ := os.Getwd() workdir, err := os.MkdirTemp("", "gopy-") @@ -1256,10 +1258,9 @@ func TestComplexGILStress(t *testing.T) { 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 { + if err := runGopy([]string{"build", "-vm=" + vm, "-output=" + workdir, "-package-prefix", "", "./_examples/simple"}); err != nil { t.Fatalf("error building simple: %v", err) } From 77954507e4b0c6761acc5f8448778e372d60bea8 Mon Sep 17 00:00:00 2001 From: b-long Date: Sat, 7 Nov 2026 14:27:32 -0500 Subject: [PATCH 5/8] chore: use master's copyright header on the branch's new files Files added on master since #409 carry "Copyright 2015-present, The go-python Authors."; match it in the three files this branch adds. --- _examples/simple/test_complex_stress.py | 2 +- bind/gen_func_test.go | 2 +- bind/symbols_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py index 80beadf3..04bd5361 100644 --- a/_examples/simple/test_complex_stress.py +++ b/_examples/simple/test_complex_stress.py @@ -1,4 +1,4 @@ -# Copyright 2026 The go-python Authors. All rights reserved. +# Copyright 2015-present, 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. diff --git a/bind/gen_func_test.go b/bind/gen_func_test.go index e9424c0b..1cb58d2f 100644 --- a/bind/gen_func_test.go +++ b/bind/gen_func_test.go @@ -1,4 +1,4 @@ -// Copyright 2026 The go-python Authors. All rights reserved. +// Copyright 2015-present, 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. diff --git a/bind/symbols_test.go b/bind/symbols_test.go index 4bb8e00a..2eff7d19 100644 --- a/bind/symbols_test.go +++ b/bind/symbols_test.go @@ -1,4 +1,4 @@ -// Copyright 2026 The go-python Authors. All rights reserved. +// Copyright 2015-present, 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. From d8a71c5861a4449aefa04ba9afd83ede73f48b76 Mon Sep 17 00:00:00 2001 From: b-long Date: Sat, 7 Nov 2026 17:57:50 -0500 Subject: [PATCH 6/8] test: cover argCallExpr's pass-through case Codecov flagged argCallExpr's default case (an argument with no py2go converter) as the only uncovered lines in the patch: no example fixture reaches it. Add a case for it, using a *C.PyObject cgoname so the test also checks that a symbol without a converter is never premarshalled. --- bind/gen_func_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/bind/gen_func_test.go b/bind/gen_func_test.go index 1cb58d2f..5b527746 100644 --- a/bind/gen_func_test.go +++ b/bind/gen_func_test.go @@ -55,6 +55,13 @@ func TestArgCallExprPrecedenceMatchesPremarshalDecision(t *testing.T) { py2goParenEx: "", } + // contrived: a *C.PyObject cgoname with no py2go converter must still + // fall through to the default case, never be premarshalled. + noConvSym := &symbol{ + goname: "int64", + cgoname: "*C.PyObject", + } + tests := []struct { name string ifchandle bool @@ -95,6 +102,12 @@ func TestArgCallExprPrecedenceMatchesPremarshalDecision(t *testing.T) { wantExpr: "C.GoString(x)", wantNeedsGIL: false, }, + { + name: "arg without a py2go conversion is passed through as-is", + sym: noConvSym, + wantExpr: "x", + wantNeedsGIL: false, + }, } g := &pyGen{shim: cpythonShim{}} From 3f803cbcce388b272f25ebfc2b5ea16cf4a76d5e Mon Sep 17 00:00:00 2001 From: b-long Date: Mon, 9 Nov 2026 21:25:21 -0500 Subject: [PATCH 7/8] catch complex arg GIL bug deterministically --- _examples/simple/test_complex_args.py | 29 ++++++++++++ _examples/simple/test_complex_stress.py | 60 ------------------------- main_test.go | 28 ++++++------ 3 files changed, 43 insertions(+), 74 deletions(-) create mode 100644 _examples/simple/test_complex_args.py delete mode 100644 _examples/simple/test_complex_stress.py diff --git a/_examples/simple/test_complex_args.py b/_examples/simple/test_complex_args.py new file mode 100644 index 00000000..71f71ddf --- /dev/null +++ b/_examples/simple/test_complex_args.py @@ -0,0 +1,29 @@ +# Copyright 2015-present, 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 test for the complex64/complex128 GIL-handling fix. +# +# A complex argument is converted with PyComplex_AsCComplex. For an exact +# `complex` that is a plain read of two doubles and works even without the +# GIL, so passing complex values cannot catch the bug. For a float or an int +# it runs Python code (the __complex__ lookup, then __float__/__index__), +# which segfaults if the GIL has already been released for the wrapped Go +# call. Each call below crashed before the fix and returns the right value +# after it. + +import simple as pkg + +cases = [ + ("float", 1.5, 2.5, 4 + 0j), + ("int", 1, 2, 3 + 0j), +] + +for name, a, b, want in cases: + for fn in (pkg.Comp64Add, pkg.Comp128Add): + got = fn(a, b) + # every expected value is exactly representable as a complex64 + if got != want: + raise AssertionError("%s(%s args) = %s, want %s" % (fn.__name__, name, got, want)) + +print("OK") diff --git a/_examples/simple/test_complex_stress.py b/_examples/simple/test_complex_stress.py deleted file mode 100644 index 04bd5361..00000000 --- a/_examples/simple/test_complex_stress.py +++ /dev/null @@ -1,60 +0,0 @@ -# Copyright 2015-present, 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 = 500 -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 % 50 == 0: - gc.collect() -finally: - STOP.set() - t.join() - -print("OK") diff --git a/main_test.go b/main_test.go index 8a8f9465..00ca9367 100644 --- a/main_test.go +++ b/main_test.go @@ -1232,21 +1232,21 @@ 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) { +// TestComplexArgGIL is a regression test for the complex64/128 GIL-handling +// fix. A complex argument is converted with PyComplex_AsCComplex, which must +// run before gopy releases the GIL for the wrapped Go call. Exact `complex` +// values cannot catch a regression, since converting one is a plain struct +// read. The test passes floats and ints instead. Converting those runs +// Python code, so the generated module segfaults if the conversion runs +// after the GIL is released. The return-side conversion +// (complex*GoToPy) takes the GIL itself and is not what this test covers. +func TestComplexArgGIL(t *testing.T) { t.Parallel() backends := []string{"py3"} for _, be := range backends { vm, ok := testBackends[be] if !ok || vm == "" { - t.Logf("Skipped testing backend %s for TestComplexGILStress\n", be) + t.Logf("Skipped testing backend %s for TestComplexArgGIL\n", be) continue } t.Run(be, func(t *testing.T) { @@ -1264,16 +1264,16 @@ func TestComplexGILStress(t *testing.T) { 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) + tstDst := filepath.Join(workdir, "test_complex_args.py") + if err := copyCmd(filepath.Join(cwd, "_examples/simple/test_complex_args.py"), tstDst); err != nil { + t.Fatalf("error copying test_complex_args.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 := exec.Command(vm, "./test_complex_args.py") cmd.Env = env cmd.Dir = workdir cmd.Stdin = os.Stdin From 7a7b4b355de0beec8adecb31a73062bc0325d0b0 Mon Sep 17 00:00:00 2001 From: b-long Date: Mon, 9 Nov 2026 21:39:22 -0500 Subject: [PATCH 8/8] add end-to-end test for callback GIL fix --- _examples/callbacks/test_callback_rval.py | 33 +++++++++++++++++++++++ main_test.go | 33 ++++++++++++++++++----- 2 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 _examples/callbacks/test_callback_rval.py diff --git a/_examples/callbacks/test_callback_rval.py b/_examples/callbacks/test_callback_rval.py new file mode 100644 index 00000000..ff3f8283 --- /dev/null +++ b/_examples/callbacks/test_callback_rval.py @@ -0,0 +1,33 @@ +# Copyright 2015-present, 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 test for the GIL handling of a Python callback's return value. +# +# When Go calls a Python callback, the generated wrapper takes the GIL, calls +# the callback, and converts the returned object to a Go value. That +# conversion, and the decref of the returned object, must both happen before +# the wrapper releases the GIL again. + +import sys + +import callbacks + +# A callback that keeps returning the same float must not leak a reference to +# it on each call. +X = float("0.25") +before = sys.getrefcount(X) +for _ in range(100): + callbacks.Apply(1.5, lambda x: X) +delta = sys.getrefcount(X) - before +if delta != 0: + raise AssertionError("refcount of the returned float grew by %d over 100 calls" % delta) + +# A float64 callback that returns an int makes the conversion +# (PyFloat_AsDouble) run int.__float__, which allocates a Python float. That +# segfaults if the GIL has already been released. +got = callbacks.Apply(1.5, lambda x: 3) +if got != 3.0: + raise AssertionError("Apply(1.5, lambda x: 3) = %r, want 3.0" % got) + +print("OK") diff --git a/main_test.go b/main_test.go index 00ca9367..aa995fea 100644 --- a/main_test.go +++ b/main_test.go @@ -1242,11 +1242,31 @@ func TestGilString(t *testing.T) { // (complex*GoToPy) takes the GIL itself and is not what this test covers. func TestComplexArgGIL(t *testing.T) { t.Parallel() + testExampleScript(t, "TestComplexArgGIL", "_examples/simple", "test_complex_args.py") +} + +// TestCallbackRvalGIL is a regression test for the GIL handling of a Python +// callback's return value. The generated wrapper (see addSignatureType in +// bind/symbols.go) must convert that value to Go and release it before +// PyGILState_Release. The test checks both: +// - a float64 callback that returns an int makes PyFloat_AsDouble run +// int.__float__, which segfaults if the GIL is already released; +// - a callback that keeps returning the same float must not raise that +// float's refcount, which it does if the result is never decref'd. +func TestCallbackRvalGIL(t *testing.T) { + t.Parallel() + testExampleScript(t, "TestCallbackRvalGIL", "_examples/callbacks", "test_callback_rval.py") +} + +// testExampleScript builds the example package at pkgPath, runs script (a +// file in that package's directory) against it with every test backend, and +// expects it to print exactly "OK". +func testExampleScript(t *testing.T, testName, pkgPath, script string) { backends := []string{"py3"} for _, be := range backends { vm, ok := testBackends[be] if !ok || vm == "" { - t.Logf("Skipped testing backend %s for TestComplexArgGIL\n", be) + t.Logf("Skipped testing backend %s for %s\n", be, testName) continue } t.Run(be, func(t *testing.T) { @@ -1260,20 +1280,19 @@ func TestComplexArgGIL(t *testing.T) { defer os.RemoveAll(workdir) writeGoMod(t, cwd, workdir) - if err := runGopy([]string{"build", "-vm=" + vm, "-output=" + workdir, "-package-prefix", "", "./_examples/simple"}); err != nil { - t.Fatalf("error building simple: %v", err) + if err := runGopy([]string{"build", "-vm=" + vm, "-output=" + workdir, "-package-prefix", "", "./" + pkgPath}); err != nil { + t.Fatalf("error building %s: %v", pkgPath, err) } - tstDst := filepath.Join(workdir, "test_complex_args.py") - if err := copyCmd(filepath.Join(cwd, "_examples/simple/test_complex_args.py"), tstDst); err != nil { - t.Fatalf("error copying test_complex_args.py: %v", err) + if err := copyCmd(filepath.Join(cwd, pkgPath, script), filepath.Join(workdir, script)); err != nil { + t.Fatalf("error copying %s: %v", script, err) } env := make([]string, len(testEnvironment)) copy(env, testEnvironment) env = append(env, fmt.Sprintf("PYTHONPATH=%s", workdir)) - cmd := exec.Command(vm, "./test_complex_args.py") + cmd := exec.Command(vm, "./"+script) cmd.Env = env cmd.Dir = workdir cmd.Stdin = os.Stdin