Skip to content

Commit d29855e

Browse files
jhonabreulclaude
andcommitted
Fix off-GIL AttributeError-hint callback: use a native __getattr__ thunk
The __getattr__ hook restored from #124 exposed the hint builder to Python as a .NET delegate (Func<PyObject, string, string>). Python invoked it through DelegateObject.tp_call -> MethodBinder.Invoke, and MethodBinder releases the GIL around every reflected invocation (allow_threads defaults to true). The callback therefore ran CPython C-API calls (GetPythonType, GetManagedString, ...) without holding the GIL on every attribute miss (hasattr, getattr with default, typos). It usually survived by luck, but segfaulted whenever the pythonnet Finalizer fired mid-callback: Finalizer.DisposeAll() starts with PyErr_Fetch, which dereferences the current thread state - NULL when the GIL is not held. This is what crashed Lean's CI test host on 2.0.55 after all 35k tests passed. Replace the Python-function-plus-delegate pair with a native method descriptor: a PyMethodDef (METH_VARARGS) around a managed thunk, turned into __getattr__ via PyDescr_NewMethod (newly bound). CPython's slot_tp_getattr_hook now calls the managed hook directly as a native method call with the GIL held - MethodBinder is never involved. The PyMethodDef and thunk are allocated once and kept for the process lifetime, since descriptors reference them and can outlive engine shutdown bookkeeping. Verified with an instrumented build (PyGILState_Check inside BuildMissingAttributeMessage): the delegate-based hook reports "GIL held: False" on every miss; this version reports "GIL held: True". Also survives a 20s stress run of concurrent attribute misses plus finalizer churn across 6 threads, and behaves identically for hasattr/ getattr-with-default, dunder probes, Python subclasses with their own __getattr__, and suggestion messages. Add a regression test asserting the installed __getattr__ is a native method_descriptor, which is the property that keeps the callback out of MethodBinder's allow-threads path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6cb81fa commit d29855e

4 files changed

Lines changed: 99 additions & 19 deletions

File tree

src/runtime/AttributeErrorHint.cs

Lines changed: 73 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
using System;
2+
using System.Reflection;
3+
using System.Runtime.InteropServices;
24

35
using Python.Runtime.Native;
46

@@ -17,44 +19,55 @@ namespace Python.Runtime
1719
/// pythonnet's metatype does not run CPython's slot-fixup machinery when an attribute
1820
/// is set on a type, so simply adding <c>__getattr__</c> to the type dict would not
1921
/// rewire the slot — we therefore wire <c>tp_getattro</c> to the hook manually.
22+
///
23+
/// The <c>__getattr__</c> itself is a native method descriptor (PyDescr_NewMethod)
24+
/// around a managed thunk, NOT a .NET delegate exposed to Python: delegate calls go
25+
/// through <see cref="MethodBinder"/>, which releases the GIL around the invocation
26+
/// (allow_threads), so the callback would run CPython C-API calls off-GIL and crash
27+
/// whenever the <see cref="Finalizer"/> fires mid-callback. The native thunk is
28+
/// called directly by the interpreter with the GIL held.
2029
/// </remarks>
2130
internal static class AttributeErrorHint
2231
{
23-
// The shared __getattr__ function object installed on every eligible type.
24-
private static PyObject? _getAttr;
25-
// The managed message builder exposed to Python, kept alive for _getAttr's globals.
26-
private static PyObject? _messageBuilder;
32+
// Unmanaged PyMethodDef backing the shared __getattr__ method descriptors.
33+
// Descriptors keep a raw pointer to it (d_method) and can outlive engine
34+
// shutdown bookkeeping, so it is allocated once and kept for the process
35+
// lifetime (as are the thunks in Interop.allocatedThunks).
36+
private static IntPtr _methodDef;
37+
// Keeps the thunk delegate for GetAttrHook alive.
38+
private static ThunkInfo? _thunk;
2739
// Address of CPython's slot_tp_getattr_hook (extracted from a probe type).
2840
private static IntPtr _hookSlot;
2941
// Address of PyObject_GenericGetAttr, used to detect types we may safely redirect.
3042
private static IntPtr _genericGetAttr;
3143

32-
private static bool IsReady => _getAttr is not null && _hookSlot != IntPtr.Zero;
44+
private static bool IsReady => _methodDef != IntPtr.Zero && _hookSlot != IntPtr.Zero;
3345

3446
internal static void Initialize()
3547
{
3648
try
3749
{
3850
_genericGetAttr = Util.ReadIntPtr(Runtime.PyBaseObjectType, TypeOffset.tp_getattro);
3951

40-
Func<PyObject, string, string> builder = ClassBase.BuildMissingAttributeMessage;
41-
_messageBuilder = builder.ToPython();
52+
if (_methodDef == IntPtr.Zero)
53+
{
54+
_thunk = Interop.GetThunk(typeof(AttributeErrorHint).GetMethod(
55+
nameof(GetAttrHook), BindingFlags.Static | BindingFlags.Public)!);
56+
IntPtr methodDef = Marshal.AllocHGlobal(4 * IntPtr.Size);
57+
TypeManager.WriteMethodDef(methodDef, "__getattr__", _thunk.Address);
58+
_methodDef = methodDef;
59+
}
4260

61+
// Define a probe class whose tp_getattro is slot_tp_getattr_hook so we
62+
// can read that function pointer.
4363
using var globals = new PyDict();
4464
Runtime.PyDict_SetItemString(globals.Reference, "__builtins__", Runtime.PyEval_GetBuiltins());
45-
globals["__clr_attr_msg__"] = _messageBuilder;
46-
47-
// Define the shared hook, plus a probe class whose tp_getattro is
48-
// slot_tp_getattr_hook so we can read that function pointer.
4965
PythonEngine.Exec(
50-
"def __clr_getattr__(self, name):\n" +
51-
" raise AttributeError(__clr_attr_msg__(self, name))\n" +
5266
"class __clr_getattr_probe__:\n" +
5367
" def __getattr__(self, name):\n" +
5468
" raise AttributeError(name)\n",
5569
globals);
5670

57-
_getAttr = globals["__clr_getattr__"];
5871
using var probe = globals["__clr_getattr_probe__"];
5972
_hookSlot = Util.ReadIntPtr(probe.Reference, TypeOffset.tp_getattro);
6073
}
@@ -86,7 +99,15 @@ internal static void Install(BorrowedReference type)
8699
return;
87100
}
88101

89-
if (Runtime.PyObject_SetAttrString(type, "__getattr__", _getAttr!.Reference) != 0)
102+
using var descr = Runtime.PyDescr_NewMethod(type, _methodDef);
103+
if (descr.IsNull())
104+
{
105+
Exceptions.Clear();
106+
return;
107+
}
108+
109+
BorrowedReference dict = Util.ReadRef(type, TypeOffset.tp_dict);
110+
if (Runtime.PyDict_SetItemString(dict, "__getattr__", descr.Borrow()) != 0)
90111
{
91112
Exceptions.Clear();
92113
return;
@@ -96,12 +117,45 @@ internal static void Install(BorrowedReference type)
96117
Runtime.PyType_Modified(type);
97118
}
98119

120+
/// <summary>
121+
/// The <c>__getattr__(self, name)</c> implementation (METH_VARARGS). CPython's
122+
/// <c>slot_tp_getattr_hook</c> only calls it after the normal lookup has failed
123+
/// and the original AttributeError has been cleared, so the full message is
124+
/// rebuilt here. Runs as a direct native method call with the GIL held.
125+
/// </summary>
126+
public static NewReference GetAttrHook(BorrowedReference ob, BorrowedReference args)
127+
{
128+
string? name = null;
129+
string message;
130+
try
131+
{
132+
if (Runtime.PyTuple_Size(args) == 1)
133+
{
134+
BorrowedReference key = Runtime.PyTuple_GetItem(args, 0);
135+
if (Runtime.PyString_Check(key))
136+
{
137+
name = Runtime.GetManagedString(key);
138+
}
139+
}
140+
141+
using var self = new PyObject(ob);
142+
message = ClassBase.BuildMissingAttributeMessage(self, name ?? "?");
143+
}
144+
catch
145+
{
146+
// Never let message building turn into a different exception.
147+
message = $"object has no attribute '{name ?? "?"}'";
148+
}
149+
150+
Exceptions.SetError(Exceptions.AttributeError, message);
151+
return default;
152+
}
153+
99154
internal static void Shutdown()
100155
{
101-
_getAttr?.Dispose();
102-
_getAttr = null;
103-
_messageBuilder?.Dispose();
104-
_messageBuilder = null;
156+
// _methodDef and _thunk are deliberately kept: method descriptors created
157+
// from them may still be reachable during interpreter teardown, and both
158+
// are reused by the next Initialize.
105159
_hookSlot = IntPtr.Zero;
106160
_genericGetAttr = IntPtr.Zero;
107161
}

src/runtime/Runtime.Delegates.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ static Delegates()
230230
PyObject_GenericGetAttr = (delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, NewReference>)GetFunctionByName(nameof(PyObject_GenericGetAttr), GetUnmanagedDll(_PythonDll));
231231
PyObject_GenericGetDict = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, NewReference>)GetFunctionByName(nameof(PyObject_GenericGetDict), GetUnmanagedDll(PythonDLL));
232232
PyObject_GenericSetAttr = (delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, BorrowedReference, int>)GetFunctionByName(nameof(PyObject_GenericSetAttr), GetUnmanagedDll(_PythonDll));
233+
PyDescr_NewMethod = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, NewReference>)GetFunctionByName(nameof(PyDescr_NewMethod), GetUnmanagedDll(_PythonDll));
233234
PyObject_GC_Del = (delegate* unmanaged[Cdecl]<StolenReference, void>)GetFunctionByName(nameof(PyObject_GC_Del), GetUnmanagedDll(_PythonDll));
234235
try
235236
{
@@ -504,6 +505,7 @@ static Delegates()
504505
internal static delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, BorrowedReference> _PyType_Lookup { get; }
505506
internal static delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, NewReference> PyObject_GenericGetAttr { get; }
506507
internal static delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, BorrowedReference, int> PyObject_GenericSetAttr { get; }
508+
internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, NewReference> PyDescr_NewMethod { get; }
507509
internal static delegate* unmanaged[Cdecl]<StolenReference, void> PyObject_GC_Del { get; }
508510
internal static delegate* unmanaged[Cdecl]<BorrowedReference, int> PyObject_GC_IsTracked { get; }
509511
internal static delegate* unmanaged[Cdecl]<BorrowedReference, void> PyObject_GC_Track { get; }

src/runtime/Runtime.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1738,6 +1738,13 @@ internal static bool PyType_IsSameAsOrSubtype(BorrowedReference type, BorrowedRe
17381738

17391739
internal static int PyObject_GenericSetAttr(BorrowedReference obj, BorrowedReference name, BorrowedReference value) => Delegates.PyObject_GenericSetAttr(obj, name, value);
17401740

1741+
1742+
/// <summary>
1743+
/// Creates a method descriptor for <paramref name="type"/> from an unmanaged
1744+
/// <c>PyMethodDef*</c>, which must remain valid for the descriptor's lifetime.
1745+
/// </summary>
1746+
internal static NewReference PyDescr_NewMethod(BorrowedReference type, IntPtr methodDef) => Delegates.PyDescr_NewMethod(type, methodDef);
1747+
17411748
internal static NewReference PyObject_GenericGetDict(BorrowedReference o) => PyObject_GenericGetDict(o, IntPtr.Zero);
17421749
internal static NewReference PyObject_GenericGetDict(BorrowedReference o, IntPtr context) => Delegates.PyObject_GenericGetDict(o, context);
17431750

tests/test_class.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,23 @@ def test_missing_attribute_hasattr_still_false():
104104
assert hasattr(s, "Length")
105105

106106

107+
def test_missing_attribute_hook_is_native():
108+
"""The __getattr__ hook must be a native method descriptor.
109+
110+
If it were a Python function calling into .NET through a delegate, the call
111+
would go through MethodBinder, which releases the GIL around the invocation:
112+
the hint-building callback would then run CPython C-API calls off-GIL and
113+
crash whenever the pythonnet Finalizer fires mid-callback (the Lean CI crash
114+
on 2.0.55).
115+
"""
116+
hook = next(
117+
c.__dict__["__getattr__"]
118+
for c in type(System.String("x")).__mro__
119+
if "__getattr__" in c.__dict__
120+
)
121+
assert type(hook).__name__ == "method_descriptor"
122+
123+
107124
def test_basic_subclass():
108125
"""Test basic subclass of a managed class."""
109126
from System.Collections import Hashtable

0 commit comments

Comments
 (0)