8000 Fix memory leak - finalizer · saaib/pythonnet@7584708 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7584708

Browse files
Martin-MolineroMartin-Molinero
committed
Fix memory leak - finalizer
- The callback set by `Runtime.Py_AddPendingCall()` was not being triggered in some cases in a multithreading environment. Replacing it with a `Task`
1 parent 928db02 commit 7584708

File tree

2 files changed

+44
-104
lines changed

2 files changed

+44
-104
lines changed

‎src/embed_tests/TestFinalizer.cs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,12 @@ public void CollectBasicObject()
3636
{
3737
Assert.IsTrue(Finalizer.Instance.Enable);
3838

39-
int thId = Thread.CurrentThread.ManagedThreadId;
4039
Finalizer.Instance.Threshold = 1;
4140
bool called = false;
41+
var objectCount = 0;
4242
EventHandler<Finalizer.CollectArgs> handler = (s, e) =>
4343
{
44-
Assert.AreEqual(thId, Thread.CurrentThread.ManagedThreadId);
45-
Assert.GreaterOrEqual(e.ObjectCount, 1);
44+
objectCount = e.ObjectCount;
4645
called = true;
4746
};
4847

@@ -73,6 +72,7 @@ public void CollectBasicObject()
7372
Finalizer.Instance.CollectOnce -= handler;
7473
}
7574
Assert.IsTrue(called);
75+
Assert.GreaterOrEqual(objectCount, 1);
7676
}
7777

7878
private static void MakeAGarbage(out WeakReference shortWeak, out WeakReference longWeak)
@@ -85,7 +85,7 @@ private static void MakeAGarbage(out WeakReference shortWeak, out WeakReference
8585

8686
private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale)
8787
{
88-
// Must larger than 512 bytes make sure Python use
88+
// Must larger than 512 bytes make sure Python use
8989
string str = new string('1', 1024);
9090
Finalizer.Instance.Enable = true;
9191
FullGCCollect();
@@ -164,10 +164,11 @@ internal static void CreateMyPyObject(IntPtr op)
164164
public void ErrorHandling()
165165
{
166166
bool called = false;
167+
var errorMessage = "";
167168
EventHandler<Finalizer.ErrorArgs> handleFunc = (sender, args) =>
168169
{
169170
called = true;
170-
Assert.AreEqual(args.Error.Message, "MyPyObject");
171+
errorMessage = args.Error.Message;
171172
};
172173
Finalizer.Instance.Threshold = 1;
173174
Finalizer.Instance.ErrorHandler += handleFunc;
@@ -193,6 +194,7 @@ public void ErrorHandling()
193194
{
194195
Finalizer.Instance.ErrorHandler -= handleFunc;
195196
}
197+
Assert.AreEqual(errorMessage, "MyPyObject");
196198
}
197199

198200
[Test]

‎src/runtime/finalizer.cs

Lines changed: 37 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
using System;
22
using System.Collections.Concurrent;
33
using System.Collections.Generic;
4-
using System.Diagnostics;
54
using System.Linq;
6-
using System.Runtime.InteropServices;
75
using System.Threading;
6+
using System.Threading.Tasks;
87

98
namespace Python.Runtime
109
{
@@ -28,20 +27,10 @@ public class ErrorArgs : EventArgs
2827
public int Threshold { get; set; }
2928
public bool Enable { get; set; }
3029

31-
[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Ansi)]
32-
struct PendingArgs
33-
{
34-
public bool cancelled;
35-
}
36-
37-
[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
38-
private delegate int PendingCall(IntPtr arg);
39-
private readonly PendingCall _collectAction;
40-
4130
private ConcurrentQueue<IPyDisposable> _objQueue = new ConcurrentQueue<IPyDisposable>();
4231
private bool _pending = false;
4332
private readonly object _collectingLock = new object();
44-
private IntPtr _pendingArgs = IntPtr.Zero;
33+
private Task _finalizerTask;
4534

4635
#region FINALIZER_CHECK
4736

@@ -84,23 +73,26 @@ private Finalizer()
8473
{
8574
Enable = true;
8675
Threshold = 200;
87-
_collectAction = OnPendingCollect;
8876
}
8977

90-
public void CallPendingFinalizers()
78+
public bool CallPendingFinalizers()
9179
{
92-
if (Thread.CurrentThread.ManagedThreadId != Runtime.MainManagedThreadId)
80+
if (Instance._finalizerTask != null
81+
&& !Instance._finalizerTask.IsCompleted)
9382
{
94-
throw new Exception("PendingCall should execute in main Python thread");
83+
var ts = PythonEngine.BeginAllowThreads();
84+
Instance._finalizerTask.Wait();
85+
PythonEngine.EndAllowThreads(ts);
86+
return true;
9587
}
96-
Runtime.Py_MakePendingCalls();
88+
return false;
9789
}
9890

9991
public void Collect()
10092
{
101-
using (var gilState = new Py.GILState())
93+
if (!Instance.CallPendingFinalizers())
10294
{
103-
DisposeAll();
95+
Instance.DisposeAll();
10496
}
10597
}
10698

@@ -141,25 +133,10 @@ internal static void Shutdown()
141133
Instance._objQueue = new ConcurrentQueue<IPyDisposable>();
142134
return;
143135
}
144-
Instance.DisposeAll();
145-
if (Thread.CurrentThread.ManagedThreadId != Runtime.MainManagedThreadId)
136+
if(!Instance.CallPendingFinalizers())
146137
{
147-
if (Instance._pendingArgs == IntPtr.Zero)
148-
{
149-
Instance.ResetPending();
150-
return;
151-
}
152-
// Not in main thread just cancel the pending operation to avoid error in different domain
153-
// It will make a memory leak
154-
unsafe
155-
{
156-
PendingArgs* args = (PendingArgs*)Instance._pendingArgs;
157-
args->cancelled = true;
158-
}
159-
Instance.ResetPending();
160-
return;
138+
Instance.DisposeAll();
161139
}
162-
Instance.CallPendingFinalizers();
163140
}
164141

165142
private void AddPendingCollect()
@@ -171,16 +148,14 @@ private void AddPendingCollect()
171148
if (!_pending)
172149
{
173150
_pending = true;
174-
var args = new PendingArgs { cancelled = false };
175-
_pendingArgs = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(PendingArgs)));
176-
Marshal.StructureToPtr(args, _pendingArgs, false);
177-
IntPtr func = Marshal.GetFunctionPointerForDelegate(_collectAction);
178-
if (Runtime.Py_AddPendingCall(func, _pendingArgs) != 0)
151+
// should already be complete but just in case
152+
_finalizerTask?.Wait();
153+
154+
_finalizerTask = Task.Factory.StartNew(() =>
179155
{
180-
// Full queue, append next time
181-
FreePendingArgs();
156+
Instance.DisposeAll();
182157
_pending = false;
183-
}
158+
});
184159
}
185160
}
186161
finally
@@ -190,29 +165,6 @@ private void AddPendingCollect()
190165
}
191166
}
192167

193-
private static int OnPendingCollect(IntPtr arg)
194-
{
195-
Debug.Assert(arg == Instance._pendingArgs);
196-
try
197-
{
198-
unsafe
199-
{
200-
PendingArgs* pendingArgs = (PendingArgs*)arg;
201-
if (pendingArgs->cancelled)
202-
{
203-
return 0;
204-
}
205-
}
206-
Instance.DisposeAll();
207-
}
208-
finally
209-
{
210-
Instance.FreePendingArgs();
211-
Instance.ResetPending();
212-
}
213-
return 0;
214-
}
215-
216168
private void DisposeAll()
217169
{
218170
CollectOnce?.Invoke(this, new CollectArgs()
@@ -223,46 +175,32 @@ private void DisposeAll()
223175
lock (_queueLock)
224176
#endif
225177
{
178+
using (Py.GIL())
179+
{
226180
#if FINALIZER_CHECK
227-
ValidateRefCount();
181+
ValidateRefCount();
228182
#endif
229-
IPyDisposable obj;
230-
while (_objQueue.TryDequeue(out obj))
231-
{
232-
try
183+
IPyDisposable obj;
184+
while (_objQueue.TryDequeue(out obj))
233185
{
234-
obj.Dispose();
235-
Runtime.CheckExceptionOccurred();
236-
}
237-
catch (Exception e)
238-
{
239-
// We should not bother the main thread
240-
ErrorHandler?.Invoke(this, new ErrorArgs()
186+
try
241187
{
242-
Error = e
243-
});
188+
obj.Dispose();
189+
Runtime.CheckExceptionOccurred();
190+
}
191+
catch (Exception e)
192+
{
193+
// We should not bother the main thread
194+
ErrorHandler?.Invoke(this, new ErrorArgs()
195+
{
196+
Error = e
197+
});
198+
}
244199
}
245200
}
246201
}
247202
}
248203

249-
private void FreePendingArgs()
250-
{
251-
if (_pendingArgs != IntPtr.Zero)
252-
{
253-
Marshal.FreeHGlobal(_pendingArgs);
254-
_pendingArgs = IntPtr.Zero;
255-
}
256-
}
257-
258-
private void ResetPending()
259-
{
260-
lock (_collectingLock)
261-
{
262-
_pending = false;
263-
}
264-
}
265-
266204
#if FINALIZER_CHECK
267205
private void ValidateRefCount()
268206
{

0 commit comments

Comments
 (0)
0