Skip to content

Commit cee8e17

Browse files
amos402amos402
committed
Add ref count check for helping discover the bugs of decref too much
1 parent eee3683 commit cee8e17

7 files changed

Lines changed: 232 additions & 25 deletions

File tree

‎src/embed_tests/TestFinalizer.cs‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ namespace Python.EmbeddingTest
88
{
99
public class TestFinalizer
1010
{
11-
private string _PYTHONMALLOC = string.Empty;
1211
private int _oldThreshold;
1312

1413
[SetUp]
@@ -195,5 +194,46 @@ public void ErrorHandling()
195194
Finalizer.Instance.ErrorHandler -= handleFunc;
196195
}
197196
}
197+
198+
[Test]
199+
public void ValidateRefCount()
200+
{
201+
if (!Finalizer.Instance.RefCountValidationEnabled)
202+
{
203+
Assert.Pass("Only run with FINALIZER_CHECK");
204+
}
205+
IntPtr ptr = IntPtr.Zero;
206+
bool called = false;
207+
Finalizer.IncorrectRefCntHandler handler = (s, e) =>
208+
{
209+
called = true;
210+
Assert.AreEqual(ptr, e.Handle);
211+
Assert.AreEqual(2, e.ImpactedObjects.Count);
212+
// Fix for this test, don't do this on general environment
213+
Runtime.Runtime.XIncref(e.Handle);
214+
return false;
215+
};
216+
Finalizer.Instance.IncorrectRefCntResovler += handler;
217+
try
218+
{
219+
ptr = CreateStringGarbage();
220+
FullGCCollect();
221+
Assert.Throws<Finalizer.IncorrectRefCountException>(() => Finalizer.Instance.Collect());
222+
Assert.IsTrue(called);
223+
}
224+
finally
225+
{
226+
Finalizer.Instance.IncorrectRefCntResovler -= handler;
227+
}
228+
}
229+
230+
private static IntPtr CreateStringGarbage()
231+
{
232+
PyString s1 = new PyString("test_string");
233+
// s2 steal a reference from s1
234+
PyString s2 = new PyString(s1.Handle);
235+
return s1.Handle;
236+
}
237+
198238
}
199239
}

‎src/runtime/Python.Runtime.15.csproj‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,10 @@
6868
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON3;$(Python3Version);$(PythonMonoDefineConstants)</DefineConstants>
6969
</PropertyGroup>
7070
<PropertyGroup Condition=" '$(Configuration)' == 'DebugMono'">
71-
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON2;$(Python2Version);$(PythonMonoDefineConstants);TRACE;DEBUG</DefineConstants>
71+
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON2;$(Python2Version);$(PythonMonoDefineConstants);FINALIZER_CHECK;TRACE;DEBUG</DefineConstants>
7272
</PropertyGroup>
7373
<PropertyGroup Condition=" '$(Configuration)' == 'DebugMonoPY3'">
74-
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON3;$(Python3Version);$(PythonMonoDefineConstants);TRACE;DEBUG</DefineConstants>
74+
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON3;$(Python3Version);$(PythonMonoDefineConstants);FINALIZER_CHECK;TRACE;DEBUG</DefineConstants>
7575
</PropertyGroup>
7676
<PropertyGroup Condition=" '$(Configuration)' == 'ReleaseWin'">
7777
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON2;$(Python2Version);$(PythonWinDefineConstants)</DefineConstants>
@@ -80,10 +80,10 @@
8080
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON3;$(Python3Version);$(PythonWinDefineConstants)</DefineConstants>
8181
</PropertyGroup>
8282
<PropertyGroup Condition=" '$(Configuration)' == 'DebugWin'">
83-
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON2;$(Python2Version);$(PythonWinDefineConstants);TRACE;DEBUG</DefineConstants>
83+
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON2;$(Python2Version);$(PythonWinDefineConstants);FINALIZER_CHECK;TRACE;DEBUG</DefineConstants>
8484
</PropertyGroup>
8585
<PropertyGroup Condition=" '$(Configuration)' == 'DebugWinPY3'">
86-
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON3;$(Python3Version);$(PythonWinDefineConstants);TRACE;DEBUG</DefineConstants>
86+
<DefineConstants Condition="'$(CustomDefineConstants)' == ''">$(DefineConstants);PYTHON3;$(Python3Version);$(PythonWinDefineConstants);FINALIZER_CHECK;TRACE;DEBUG</DefineConstants>
8787
</PropertyGroup>
8888

8989
<ItemGroup Condition=" '$(PythonInteropFile)' != '' ">

‎src/runtime/delegatemanager.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ A possible alternate strategy would be to create custom subclasses
181181
too "special" for this to work. It would be more work, so for now
182182
the 80/20 rule applies :) */
183183

184-
public class Dispatcher : IDisposable
184+
public class Dispatcher : IPyDisposable
185185
{
186186
public IntPtr target;
187187
public Type dtype;
@@ -276,6 +276,11 @@ public object TrueDispatch(ArrayList args)
276276
Runtime.XDecref(op);
277277
return result;
278278
}
279+
280+
public IntPtr[] GetTrackedHandles()
281+
{
282+
return new IntPtr[] { target };
283+
}
279284
}
280285

281286

‎src/runtime/finalizer.cs‎

Lines changed: 141 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,48 @@ struct PendingArgs
3838
private delegate int PendingCall(IntPtr arg);
3939
private readonly PendingCall _collectAction;
4040

41-
private ConcurrentQueue<IDisposable> _objQueue = new ConcurrentQueue<IDisposable>();
41+
private ConcurrentQueue<IPyDisposable> _objQueue = new ConcurrentQueue<IPyDisposable>();
4242
private bool _pending = false;
4343
private readonly object _collectingLock = new object();
4444
private IntPtr _pendingArgs;
4545

46+
#region FINALIZER_CHECK
47+
48+
#if FINALIZER_CHECK
49+
private readonly object _queueLock = new object();
50+
public bool RefCountValidationEnabled { get; set; } = true;
51+
#else
52+
public readonly bool RefCountValidationEnabled = false;
53+
#endif
54+
// Keep these declarations for compat even no FINALIZER_CHECK
55+
public class IncorrectFinalizeArgs : EventArgs
56+
{
57+
public IntPtr Handle { get; internal set; }
58+
public ICollection<IPyDisposable> ImpactedObjects { get; internal set; }
59+
}
60+
61+
public class IncorrectRefCountException : Exception
62+
{
63+
public IntPtr PyPtr { get; internal set; }
64+
private string _message;
65+
public override string Message => _message;
66+
67+
public IncorrectRefCountException(IntPtr ptr)
68+
{
69+
PyPtr = ptr;
70+
IntPtr pyname = Runtime.PyObject_Unicode(PyPtr);
71+
string name = Runtime.GetManagedString(pyname);
72+
Runtime.XDecref(pyname);
73+
_message = $"{name} may has a incorrect ref count";
74+
}
75+
}
76+
77+
public delegate bool IncorrectRefCntHandler(object sender, IncorrectFinalizeArgs e);
78+
public event IncorrectRefCntHandler IncorrectRefCntResovler;
79+
public bool ThrowIfUnhandleIncorrectRefCount { get; set; } = true;
80+
81+
#endregion
82+
4683
private Finalizer()
4784
{
4885
Enable = true;
@@ -72,7 +109,7 @@ public List<WeakReference> GetCollectedObjects()
72109
return _objQueue.Select(T => new WeakReference(T)).ToList();
73110
}
74111

75-
internal void AddFinalizedObject(IDisposable obj)
112+
internal void AddFinalizedObject(IPyDisposable obj)
76113
{
77114
if (!Enable)
78115
{
@@ -84,7 +121,12 @@ internal void AddFinalizedObject(IDisposable obj)
84121
// for avoiding that case, user should call GC.Collect manual before shutdown.
85122
return;
86123
}
87-
_objQueue.Enqueue(obj);
124+
#if FINALIZER_CHECK
125+
lock (_queueLock)
126+
#endif
127+
{
128+
_objQueue.Enqueue(obj);
129+
}
88130
GC.ReRegisterForFinalize(obj);
89131
if (_objQueue.Count >= Threshold)
90132
{
@@ -96,7 +138,7 @@ internal static void Shutdown()
96138
{
97139
if (Runtime.Py_IsInitialized() == 0)
98140
{
99-
Instance._objQueue = new ConcurrentQueue<IDisposable>();
141+
Instance._objQueue = new ConcurrentQueue<IPyDisposable>();
100142
return;
101143
}
102144
Instance.DisposeAll();
@@ -175,21 +217,29 @@ private void DisposeAll()
175217
{
176218
ObjectCount = _objQueue.Count
177219
});
178-
IDisposable obj;
179-
while (_objQueue.TryDequeue(out obj))
220+
#if FINALIZER_CHECK
221+
lock (_queueLock)
222+
#endif
180223
{
181-
try
182-
{
183-
obj.Dispose();
184-
Runtime.CheckExceptionOccurred();
185-
}
186-
catch (Exception e)
224+
#if FINALIZER_CHECK
225+
ValidateRefCount();
226+
#endif
227+
IPyDisposable obj;
228+
while (_objQueue.TryDequeue(out obj))
187229
{
188-
// We should not bother the main thread
189-
ErrorHandler?.Invoke(this, new ErrorArgs()
230+
try
231+
{
232+
obj.Dispose();
233+
Runtime.CheckExceptionOccurred();
234+
}
235+
catch (Exception e)
190236
{
191-
Error = e
192-
});
237+
// We should not bother the main thread
238+
ErrorHandler?.Invoke(this, new ErrorArgs()
239+
{
240+
Error = e
241+
});
242+
}
193243
}
194244
}
195245
}
@@ -202,5 +252,80 @@ private void ResetPending()
202252
_pendingArgs = IntPtr.Zero;
203253
}
204254
}
255+
256+
#if FINALIZER_CHECK
257+
private void ValidateRefCount()
258+
{
259+
if (!RefCountValidationEnabled)
260+
{
261+
return;
262+
}
263+
var counter = new Dictionary<IntPtr, long>();
264+
var holdRefs = new Dictionary<IntPtr, long>();
265+
var indexer = new Dictionary<IntPtr, List<IPyDisposable>>();
266+
foreach (var obj in _objQueue)
267+
{
268+
IntPtr[] handles = obj.GetTrackedHandles();
269+
foreach (var handle in handles)
270+
{
271+
if (handle == IntPtr.Zero)
272+
{
273+
continue;
274+
}
275+
if (!counter.ContainsKey(handle))
276+
{
277+
counter[handle] = 0;
278+
}
279+
counter[handle]++;
280+
if (!holdRefs.ContainsKey(handle))
281+
{
282+
holdRefs[handle] = Runtime.Refcount(handle);
283+
}
284+
List<IPyDisposable> objs;
285+
if (!indexer.TryGetValue(handle, out objs))
286+
{
287+
objs = new List<IPyDisposable>();
288+
indexer.Add(handle, objs);
289+
}
290+
objs.Add(obj);
291+
}
292+
}
293+
foreach (var pair in counter)
294+
{
295+
IntPtr handle = pair.Key;
296+
long cnt = pair.Value;
297+
// Tracked handle's ref count is larger than the object's holds
298+
// it may take an unspecified behaviour if it decref in Dispose
299+
if (cnt > holdRefs[handle])
300+
{
301+
var args = new IncorrectFinalizeArgs()
302+
{
303+
Handle = handle,
304+
ImpactedObjects = indexer[handle]
305+
};
306+
bool handled = false;
307+
if (IncorrectRefCntResovler != null)
308+
{
309+
var funcList = IncorrectRefCntResovler.GetInvocationList();
310+
foreach (IncorrectRefCntHandler func in funcList)
311+
{
312+
if (func(this, args))
313+
{
314+
handled = true;
315+
break;
316+
}
317+
}
318+
}
319+
if (!handled && ThrowIfUnhandleIncorrectRefCount)
320+
{
321+
throw new IncorrectRefCountException(handle);
322+
}
323+
}
324+
// Make sure no other references for PyObjects after this method
325+
indexer[handle].Clear();
326+
}
327+
indexer.Clear();
328+
}
329+
#endif
205330
}
206331
}

‎src/runtime/pyobject.cs‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,33 @@
11
using System;
22
using System.Collections;
33
using System.Collections.Generic;
4+
using System.Diagnostics;
45
using System.Dynamic;
56
using System.Linq.Expressions;
67

78
namespace Python.Runtime
89
{
10+
public interface IPyDisposable : IDisposable
11+
{
12+
IntPtr[] GetTrackedHandles();
13+
}
14+
915
/// <summary>
1016
/// Represents a generic Python object. The methods of this class are
1117
/// generally equivalent to the Python "abstract object API". See
1218
/// PY2: https://docs.python.org/2/c-api/object.html
1319
/// PY3: https://docs.python.org/3/c-api/object.html
1420
/// for details.
1521
/// </summary>
16-
public class PyObject : DynamicObject, IEnumerable, IDisposable
22+
public class PyObject : DynamicObject, IEnumerable, IPyDisposable
1723
{
24+
#if TRACE_ALLOC
25+
/// <summary>
26+
/// Trace stack for PyObject's construction
27+
/// </summary>
28+
public StackTrace Traceback { get; private set; }
29+
#endif
30+
1831
protected internal IntPtr obj = IntPtr.Zero;
1932
private bool disposed = false;
2033
private bool _finalized = false;
@@ -31,19 +44,29 @@ public class PyObject : DynamicObject, IEnumerable, IDisposable
3144
public PyObject(IntPtr ptr)
3245
{
3346
obj = ptr;
47+
#if TRACE_ALLOC
48+
Traceback = new StackTrace(1);
49+
#endif
3450
}
3551

3652
// Protected default constructor to allow subclasses to manage
3753
// initialization in different ways as appropriate.
3854

3955
protected PyObject()
4056
{
57+
#if TRACE_ALLOC
58+
Traceback = new StackTrace(1);
59+
#endif
4160
}
4261

4362
// Ensure that encapsulated Python object is decref'ed appropriately
4463
// when the managed wrapper is garbage-collected.
4564
~PyObject()
4665
{
66+
if (obj == IntPtr.Zero)
67+
{
68+
return;
69+
}
4770
if (_finalized || disposed)
4871
{
4972
return;
@@ -158,6 +181,10 @@ public void Dispose()
158181
GC.SuppressFinalize(this);
159182
}
160183

184+
public IntPtr[] GetTrackedHandles()
185+
{
186+
return new IntPtr[] { obj };
187+
}
161188

162189
/// <summary>
163190
/// GetPythonType Method

0 commit comments

Comments
 (0)