[b-long]

Motivated by feedback from @satarsa (see PR 399 feedback).

Relates-to: #400

[satarsa]

Had a look. The argument side looks right to me, the tuple side I would drop, and there is a third window neither PR touches that I think is the better target. Everything below is against #399 head (202b0fb).

The argument side is right. Hoisting the *C.PyObject marshalling above PyEval_SaveThread is the right shape. Keying needsGILForArgMarshal off cgoname == "*C.PyObject" rather than off complex specifically is a good call: bind/stdtypes.go gives that cgoname to exactly complex64/complex128 today, so anything added later with Go-side marshalling is covered automatically. Excluding isSignature() is also correct, that closure takes the GIL itself.

And to confirm what you found: reverting d41db33 on top of this cannot work. C.PyEval_RestoreThread is deferred, so the whole return complex64GoToPy(pkg.Comp64Add(...)) expression, allocation included, is evaluated while the GIL is still released. Argument side and return side are two separate windows and both need covering.

One nit. The premarshal variable is declared in a new loop over args and consumed in the pre-existing switch in a second loop, and in that switch the ifchandle && arg.sym.goname == "interface{}" and arg.sym.isSignature() cases come before premarshalled[i]. If any argument ever matches both an earlier case and needsGILForArgMarshal, the result is _premarshalN declared and not used. Nothing reaches that today, but it is an unpleasant failure mode because it surfaces as a compile error in the user's generated package rather than in gopy. Computing na once in the existing switch and hoisting that expression when the symbol needs the GIL would make it unrepresentable rather than merely unreachable.

The tuple side: I would drop it. buildTuple's only caller already holds the GIL across the whole region. In addSignatureType (bind/symbols.go, ~1086-1118 at #399 head):

_gstate := C.PyGILState_Ensure()      // 1086
...
<buildTuple output spliced here>      // 1110
C.PyObject_CallObject(_fun_arg, _fcargs)   // 1111
C.gopy_decref(_fcargs)                // 1112
C.gopy_err_handle()                   // 1117
C.PyGILState_Release(_gstate)         // 1118

So the new inner PyGILState_Ensure/Release is a no-op today. Nesting is safe, so it is harmless, but it is not buying anything.

As future-proofing it also does not do what the comment says. The tuple is created inside the new bracket and used outside it: PyObject_CallObject and gopy_decref are emitted by the caller, after the inner Release. A future caller that got the GIL wrong would still be wrong. It would just stop faulting at PyTuple_New, which is the most diagnosable point it had.

TestBuildTupleHoldsGIL then pins that down as an invariant. The requirement is not "the tuple is built under the GIL", it is "the tuple is built, filled, passed to CallObject and decref'd under the GIL", and the first can hold while the second is violated. A test that can pass in the broken case is worse than no test, because the next person reads it as a guarantee.

If you want that contract enforced rather than conventional, the way to get it is to emit the whole Ensure -> build -> call -> decref -> Release region from one place, so a caller has no opportunity to bracket it wrong. Otherwise I would leave buildTuple as it was and keep the reasoning as a comment on the caller.

Minor, separate from the GIL question: wrapping the body in { ... } narrows the scope of anything the loop declares. Nothing escapes today (the loop only emits PyTuple_SetItem calls), so it compiles, but it is a trap for whoever next adds a temporary in there.

Where the live GIL-less window actually is

Still in addSignatureType, the Release on line 1118 happens before the return value is converted on 1120-1124:

C.PyGILState_Release(_gstate)                 // 1118
...
return C.GoString(C.PyBytes_AsString(_fcret)) // via pyObjectToGo, 1120-1124

so C.PyLong_AsLongLong(_fcret) / C.PyFloat_AsDouble(_fcret) / C.PyBytes_AsString(_fcret) all touch a PyObject with no GIL held, on a path that is live and covered by tests. It does not fault today because none of those allocate, which is why _examples/gilstring stays green on the 3.12 leg, but it is the same UB shape as #400 and unlike buildTuple it is actually reachable. Moving the Release to after the conversion is a two-line change.

While in there: _fcargs is decref'd on 1112 but _fcret never is, and PyObject_CallObject returns a new reference, so unless I am missing a decref elsewhere every callback with a return value leaks one object per call. Unrelated to the GIL, just adjacent.

The stress test

Well aimed. The churn thread can only run while the main thread has released the GIL, which is exactly gopy's SaveThread window, so the pressure lands where it matters rather than somewhere decorative. One nit: pre-fix the crash is deterministic on the very first call, so 20000 iterations across a 24-job matrix buys wall clock rather than detection. A few hundred would read the same.

[b-long]

Thank you again for the review and collaboration, @satarsa! 🙇

Good catch on the tuple side — you were totally right. addSignatureType was already holding the GIL across the whole build/call/decref sequence, so that bracket in buildTuple was redundant. I pulled it back out and left a comment on the caller to clarify the reasoning. The test that went with it was only looking at the tuple-build fragment in isolation, so I swapped it for one that checks that the full closure stays inside a single Ensure/Release.

That GIL-less window you flagged in addSignatureType was a great spot. The release was happening before converting the return value from _fcret. Fixed that up, and noticed _fcret wasn't being decref'd either, so that's a nice extra leak closed!

For the gen_func.go nit, instead of just reordering the switch, I moved the logic for picking the expression (and whether it needs the GIL) into argCallExpr. Now both the premarshal and call-argument loops call it with the same inputs, which makes that failure mode impossible by construction rather than just unreachable. Added a test covering the interface{}/*C.PyObject collision you sketched.

Also trimmed the stress test down from 20,000 iterations to 500 — good point that the crash is deterministic on the first call anyway.

Everything's updated in e24deba. I squashed the branch down to two commits: ec3b50c is the state from your original review, and e24deba includes all the updates above (so the old comment thread will show as outdated against the new diff).

[codecov]

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.38%. Comparing base (acffd5c) to head (7795450).

Files with missing lines	Patch %	Lines
bind/gen_func.go	92.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #401      +/-   ##
==========================================
+ Coverage   89.34%   89.38%   +0.03%     
==========================================
  Files          33       33              
  Lines        4929     4947      +18     
==========================================
+ Hits         4404     4422      +18     
  Misses        525      525              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.