Skip to content

Commit 610d309

Browse files
lostmsulostmsu
committed
simplified Finalizer
This is an attempt to make finalizer much simpler. Instead of using .NET async tasks, Py_AddPendingCall or other synchronization mechanism we simply move objects to be finalized into a ConcurrentQueue, which is periodically drained when a new PyObject is constructed.
1 parent 2910074 commit 610d309

5 files changed

Lines changed: 50 additions & 83 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].
2727
- When calling C# from Python, enable passing argument of any type to a parameter of C# type `object` by wrapping it into `PyObject` instance. ([#881][i881])
2828
- Added support for kwarg parameters when calling .NET methods from Python
2929
- Changed method for finding MSBuild using vswhere
30+
- Reworked `Finalizer`. Now objects drop into its queue upon finalization, which is periodically drained when new objects are created.
3031

3132
### Fixed
3233

‎src/embed_tests/TestFinalizer.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public void CollectBasicObject()
7777
}
7878
try
7979
{
80-
Finalizer.Instance.Collect(forceDispose: false);
80+
Finalizer.Instance.Collect();
8181
}
8282
finally
8383
{

‎src/runtime/finalizer.cs‎

Lines changed: 33 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,7 @@ public class ErrorArgs : EventArgs
2828
public bool Enable { get; set; }
2929

3030
private ConcurrentQueue<IPyDisposable> _objQueue = new ConcurrentQueue<IPyDisposable>();
31-
private bool _pending = false;
32-
private readonly object _collectingLock = new object();
33-
private Task _finalizerTask;
31+
private int _throttled;
3432

3533
#region FINALIZER_CHECK
3634

@@ -75,19 +73,16 @@ private Finalizer()
7573
Threshold = 200;
7674
}
7775

78-
public void Collect(bool forceDispose = true)
76+
[Obsolete("forceDispose parameter is unused. All objects are disposed regardless.")]
77+
public void Collect(bool forceDispose) => this.DisposeAll();
78+
public void Collect() => this.DisposeAll();
79+
80+
internal void ThrottledCollect()
7981
{
80-
if (Instance._finalizerTask != null
81-
&& !Instance._finalizerTask.IsCompleted)
82-
{
83-
var ts = PythonEngine.BeginAllowThreads();
84-
Instance._finalizerTask.Wait();
85-
PythonEngine.EndAllowThreads(ts);
86-
}
87-
else if (forceDispose)
88-
{
89-
Instance.DisposeAll();
90-
}
82+
_throttled = unchecked(this._throttled + 1);
83+
if (!Enable || _throttled < Threshold) return;
84+
_throttled = 0;
85+
this.Collect();
9186
}
9287

9388
public List<WeakReference> GetCollectedObjects()
@@ -101,62 +96,18 @@ internal void AddFinalizedObject(IPyDisposable obj)
10196
{
10297
return;
10398
}
104-
if (Runtime.Py_IsInitialized() == 0)
105-
{
106-
// XXX: Memory will leak if a PyObject finalized after Python shutdown,
107-
// for avoiding that case, user should call GC.Collect manual before shutdown.
108-
return;
109-
}
99+
110100
#if FINALIZER_CHECK
111101
lock (_queueLock)
112102
#endif
113103
{
114-
_objQueue.Enqueue(obj);
115-
}
116-
GC.ReRegisterForFinalize(obj);
117-
if (!_pending && _objQueue.Count >= Threshold)
118-
{
119-
AddPendingCollect();
104+
this._objQueue.Enqueue(obj);
120105
}
121106
}
122107

123108
internal static void Shutdown()
124109
{
125-
if (Runtime.Py_IsInitialized() == 0)
126-
{
127-
Instance._objQueue = new ConcurrentQueue<IPyDisposable>();
128-
return;
129-
}
130-
Instance.Collect(forceDispose: true);
131-
}
132-
133-
private void AddPendingCollect()
134-
{
135-
if(Monitor.TryEnter(_collectingLock))
136-
{
137-
try
138-
{
139-
if (!_pending)
140-
{
141-
_pending = true;
142-
// should already be complete but just in case
143-
_finalizerTask?.Wait();
144-
145-
_finalizerTask = Task.Factory.StartNew(() =>
146-
{
147-
using (Py.GIL())
148-
{
149-
Instance.DisposeAll();
150-
_pending = false;
151-
}
152-
});
153-
}
154-
}
155-
finally
156-
{
157-
Monitor.Exit(_collectingLock);
158-
}
159-
}
110+
Instance.DisposeAll();
160111
}
161112

162113
private void DisposeAll()
@@ -178,12 +129,18 @@ private void DisposeAll()
178129
try
179130
{
180131
obj.Dispose();
181-
Runtime.CheckExceptionOccurred();
182132
}
183133
catch (Exception e)
184134
{
185-
// We should not bother the main thread
186-
ErrorHandler?.Invoke(this, new ErrorArgs()
135+
var handler = ErrorHandler;
136+
if (handler is null)
137+
{
138+
throw new FinalizationException(
139+
"Python object finalization failed",
140+
disposable: obj, innerException: e);
141+
}
142+
143+
handler.Invoke(this, new ErrorArgs()
187144
{
188145
Error = e
189146
});
@@ -267,4 +224,15 @@ private void ValidateRefCount()
267224
}
268225
#endif
269226
}
227+
228+
public class FinalizationException : Exception
229+
{
230+
public IPyDisposable Disposable { get; }
231+
232+
public FinalizationException(string message, IPyDisposable disposable, Exception innerException)
233+
: base(message, innerException)
234+
{
235+
this.Disposable = disposable ?? throw new ArgumentNullException(nameof(disposable));
236+
}
237+
}
270238
}

‎src/runtime/pyobject.cs‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@ public class PyObject : DynamicObject, IEnumerable, IPyDisposable
3030
#endif
3131

3232
protected internal IntPtr obj = IntPtr.Zero;
33-
private bool disposed = false;
34-
private bool _finalized = false;
3533

3634
internal BorrowedReference Reference => new BorrowedReference(obj);
3735

@@ -49,6 +47,7 @@ public PyObject(IntPtr ptr)
4947
if (ptr == IntPtr.Zero) throw new ArgumentNullException(nameof(ptr));
5048

5149
obj = ptr;
50+
Finalizer.Instance.ThrottledCollect();
5251
#if TRACE_ALLOC
5352
Traceback = new StackTrace(1);
5453
#endif
@@ -64,6 +63,7 @@ internal PyObject(BorrowedReference reference)
6463
if (reference.IsNull) throw new ArgumentNullException(nameof(reference));
6564

6665
obj = Runtime.SelfIncRef(reference.DangerousGetAddress());
66+
Finalizer.Instance.ThrottledCollect();
6767
#if TRACE_ALLOC
6868
Traceback = new StackTrace(1);
6969
#endif
@@ -74,6 +74,7 @@ internal PyObject(BorrowedReference reference)
7474
[Obsolete("Please, always use PyObject(*Reference)")]
7575
protected PyObject()
7676
{
77+
Finalizer.Instance.ThrottledCollect();
7778
#if TRACE_ALLOC
7879
Traceback = new StackTrace(1);
7980
#endif
@@ -87,12 +88,6 @@ protected PyObject()
8788
{
8889
return;
8990
}
90-
if (_finalized || disposed)
91-
{
92-
return;
93-
}
94-
// Prevent a infinity loop by calling GC.WaitForPendingFinalizers
95-
_finalized = true;
9691
Finalizer.Instance.AddFinalizedObject(this);
9792
}
9893

@@ -183,17 +178,19 @@ public T As<T>()
183178
/// </remarks>
184179
protected virtual void Dispose(bool disposing)
185180
{
186-
if (!disposed)
181+
if (this.obj == IntPtr.Zero)
187182
{
188-
if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
189-
{
190-
IntPtr gs = PythonEngine.AcquireLock();
191-
Runtime.XDecref(obj);
192-
obj = IntPtr.Zero;
193-
PythonEngine.ReleaseLock(gs);
194-
}
195-
disposed = true;
183+
return;
184+
}
185+
186+
if (Runtime.Py_IsInitialized() == 0)
187+
throw new InvalidOperationException("Python runtime must be initialized");
188+
189+
if (!Runtime.IsFinalizing)
190+
{
191+
Runtime.XDecref(this.obj);
196192
}
193+
this.obj = IntPtr.Zero;
197194
}
198195

199196
public void Dispose()

‎src/runtime/pythonengine.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ public static string PythonHome
8080
}
8181
set
8282
{
83+
// this value is null in the beginning
8384
Marshal.FreeHGlobal(_pythonHome);
8485
_pythonHome = UcsMarshaler.Py3UnicodePy2StringtoPtr(value);
8586
Runtime.Py_SetPythonHome(_pythonHome);

0 commit comments

Comments
 (0)