Skip to content

Commit 411fcce

Browse files
jhonabreulclaude
andcommitted
Honor ForbidPythonThreadsAttribute when binding methods (fix GC crash)
MethodObject always constructed its binder with allow_threads = true (the default), ignoring [ForbidPythonThreads]. The per-method check that upstream performs (MethodObject.AllowThreads) had been dropped, leaving only a "TODO: ForbidPythonThreadsAttribute per method info" comment. As a result, methods marked [ForbidPythonThreads] - e.g. Runtime.TryCollectingGarbage - released the GIL (PythonEngine.BeginAllowThreads) around their invocation. Calling the CPython C-API without the GIL corrupts the interpreter, so the very first PyGC_Collect() inside TryCollectingGarbage faulted with an access violation (0xC0000005), crashing the host. This is why test_constructors.py::test_constructor_leak aborted the whole pytest run while a plain Python gc.collect() (GIL held) was fine. Port the upstream behavior: compute allow_threads from ForbidPythonThreadsAttribute on the overloads so such methods keep the GIL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f95650f commit 411fcce

1 file changed

Lines changed: 35 additions & 4 deletions

File tree

src/runtime/Types/MethodObject.cs

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ namespace Python.Runtime
1313
/// Implements a Python type that represents a CLR method. Method objects
1414
/// support a subscript syntax [] to allow explicit overload selection.
1515
/// </summary>
16-
/// <remarks>
17-
/// TODO: ForbidPythonThreadsAttribute per method info
18-
/// </remarks>
1916
[Serializable]
2017
internal class MethodObject : ExtensionType
2118
{
@@ -30,7 +27,7 @@ internal class MethodObject : ExtensionType
3027
internal PyString? doc;
3128
internal MaybeType type;
3229

33-
public MethodObject(MaybeType type, string name, List<MethodInformation> info, bool allow_threads = MethodBinder.DefaultAllowThreads)
30+
public MethodObject(MaybeType type, string name, List<MethodInformation> info, bool allow_threads)
3431
{
3532
this.type = type;
3633
this.name = name;
@@ -42,6 +39,40 @@ public MethodObject(MaybeType type, string name, List<MethodInformation> info, b
4239
is_static = info.Any(x => x.MethodBase.IsStatic);
4340
}
4441

42+
public MethodObject(MaybeType type, string name, List<MethodInformation> info)
43+
: this(type, name, info, allow_threads: AllowThreads(info))
44+
{
45+
}
46+
47+
/// <summary>
48+
/// Determines whether the Python GIL should be released around invocations
49+
/// of these overloads, based on the <see cref="ForbidPythonThreadsAttribute"/>.
50+
/// Methods that call back into the CPython C-API (e.g. those marked with the
51+
/// attribute) must keep the GIL held; otherwise the call corrupts the
52+
/// interpreter / crashes.
53+
/// </summary>
54+
static bool AllowThreads(List<MethodInformation> methods)
55+
{
56+
bool hasAllowOverload = false, hasForbidOverload = false;
57+
foreach (var method in methods)
58+
{
59+
bool forbidsThreads = method.MethodBase.GetCustomAttribute<ForbidPythonThreadsAttribute>(inherit: false) != null;
60+
if (forbidsThreads)
61+
{
62+
hasForbidOverload = true;
63+
}
64+
else
65+
{
66+
hasAllowOverload = true;
67+
}
68+
}
69+
70+
if (hasAllowOverload && hasForbidOverload)
71+
throw new NotImplementedException("All method overloads currently must either allow or forbid Python threads together");
72+
73+
return !hasForbidOverload;
74+
}
75+
4576
public bool IsInstanceConstructor => name == "__init__";
4677

4778
public MethodObject WithOverloads(List<MethodInformation> overloads)

0 commit comments

Comments
 (0)