Merge pull request #2386 from Frawak/fix/1977-access-violation-gc

lostmsu · web-flow · commit ea059ca0aeec · 2024-08-06T15:12:49.000-07:00
Fix access violation exception on shutdown
diff --git a/AUTHORS.md b/AUTHORS.md
@@ -38,6 +38,7 @@
 -   Dmitriy Se ([@dmitriyse](https://github.com/dmitriyse))
 -   Félix Bourbonnais ([@BadSingleton](https://github.com/BadSingleton))
 -   Florian Treurniet ([@ftreurni](https://github.com/ftreurni))
+-   Frank Witscher ([@Frawak](https://github.com/Frawak))
 -   He-chien Tsai ([@t3476](https://github.com/t3476))
 -   Inna Wiesel ([@inna-w](https://github.com/inna-w))
 -   Ivan Cronyn ([@cronan](https://github.com/cronan))
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -24,6 +24,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].
 -   Fixed RecursionError for reverse operators on C# operable types from python. See #2240
 -   Fixed crash when .NET event has no `AddMethod`
 -   Fixed probing for assemblies in `sys.path` failing when a path in `sys.path` has invalid characters. See #2376
+-   Fixed possible access violation exception on shutdown. See ([#1977][i1977])
 
 ## [3.0.3](https://github.com/pythonnet/pythonnet/releases/tag/v3.0.3) - 2023-10-11
 
@@ -971,3 +972,4 @@ This version improves performance on benchmarks significantly compared to 2.3.
 [i1481]: https://github.com/pythonnet/pythonnet/issues/1481
 [i1672]: https://github.com/pythonnet/pythonnet/pull/1672
 [i2311]: https://github.com/pythonnet/pythonnet/issues/2311
+[i1977]: https://github.com/pythonnet/pythonnet/issues/1977
diff --git a/src/runtime/Finalizer.cs b/src/runtime/Finalizer.cs
@@ -191,7 +191,7 @@ internal static void Shutdown()
             Instance.started = false;
         }
 
-        internal nint DisposeAll()
+        internal nint DisposeAll(bool disposeObj = true, bool disposeDerived = true, bool disposeBuffer = true)
         {
             if (_objQueue.IsEmpty && _derivedQueue.IsEmpty && _bufferQueue.IsEmpty)
                 return 0;
@@ -216,7 +216,7 @@ internal nint DisposeAll()
 
                 try
                 {
-                    while (!_objQueue.IsEmpty)
+                    if (disposeObj) while (!_objQueue.IsEmpty)
                     {
                         if (!_objQueue.TryDequeue(out var obj))
                             continue;
@@ -240,7 +240,7 @@ internal nint DisposeAll()
                         }
                     }
 
-                    while (!_derivedQueue.IsEmpty)
+                    if (disposeDerived) while (!_derivedQueue.IsEmpty)
                     {
                         if (!_derivedQueue.TryDequeue(out var derived))
                             continue;
@@ -258,7 +258,7 @@ internal nint DisposeAll()
                         collected++;
                     }
 
-                    while (!_bufferQueue.IsEmpty)
+                    if (disposeBuffer) while (!_bufferQueue.IsEmpty)
                     {
                         if (!_bufferQueue.TryDequeue(out var buffer))
                             continue;
diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs
@@ -158,6 +158,7 @@ internal static void Initialize(bool initSigs = false)
             ClassManager.Reset();
             ClassDerivedObject.Reset();
             TypeManager.Initialize();
+            CLRObject.creationBlocked = false;
             _typesInitialized = true;
 
             // Initialize modules that depend on the runtime class.
@@ -278,6 +279,10 @@ internal static void Shutdown()
             ClearClrModules();
             RemoveClrRootModule();
 
+            TryCollectingGarbage(MaxCollectRetriesOnShutdown, forceBreakLoops: true,
+                                 obj: true, derived: false, buffer: false);
+            CLRObject.creationBlocked = true;
+
             NullGCHandles(ExtensionType.loadedExtensions);
             ClassManager.RemoveClasses();
             TypeManager.RemoveTypes();
@@ -295,8 +300,7 @@ internal static void Shutdown()
             PyObjectConversions.Reset();
 
             PyGC_Collect();
-            bool everythingSeemsCollected = TryCollectingGarbage(MaxCollectRetriesOnShutdown,
-                                                                 forceBreakLoops: true);
+            bool everythingSeemsCollected = TryCollectingGarbage(MaxCollectRetriesOnShutdown);
             Debug.Assert(everythingSeemsCollected);
 
             Finalizer.Shutdown();
@@ -328,7 +332,8 @@ internal static void Shutdown()
 
         const int MaxCollectRetriesOnShutdown = 20;
         internal static int _collected;
-        static bool TryCollectingGarbage(int runs, bool forceBreakLoops)
+        static bool TryCollectingGarbage(int runs, bool forceBreakLoops,
+                                         bool obj = true, bool derived = true, bool buffer = true)
         {
             if (runs <= 0) throw new ArgumentOutOfRangeException(nameof(runs));
 
@@ -341,7 +346,9 @@ static bool TryCollectingGarbage(int runs, bool forceBreakLoops)
                     GC.Collect();
                     GC.WaitForPendingFinalizers();
                     pyCollected += PyGC_Collect();
-                    pyCollected += Finalizer.Instance.DisposeAll();
+                    pyCollected += Finalizer.Instance.DisposeAll(disposeObj: obj,
+                                                                 disposeDerived: derived,
+                                                                 disposeBuffer: buffer);
                 }
                 if (Volatile.Read(ref _collected) == 0 && pyCollected == 0)
                 {
diff --git a/src/runtime/Types/ClrObject.cs b/src/runtime/Types/ClrObject.cs
@@ -11,10 +11,15 @@ internal sealed class CLRObject : ManagedType
     {
         internal readonly object inst;
 
+        internal static bool creationBlocked = false;
+
         // "borrowed" references
         internal static readonly HashSet<IntPtr> reflectedObjects = new();
         static NewReference Create(object ob, BorrowedReference tp)
         {
+            if (creationBlocked)
+                throw new InvalidOperationException("Reflected objects should not be created anymore.");
+
             Debug.Assert(tp != null);
             var py = Runtime.PyType_GenericAlloc(tp, 0);
 
@@ -61,6 +66,9 @@ internal static void Restore(object ob, BorrowedReference pyHandle, Dictionary<s
 
         protected override void OnLoad(BorrowedReference ob, Dictionary<string, object?>? context)
         {
+            if (creationBlocked)
+                throw new InvalidOperationException("Reflected objects should not be loaded anymore.");
+
             base.OnLoad(ob, context);
             GCHandle gc = GCHandle.Alloc(this);
             SetGCHandle(ob, gc);

Original file line number	Diff line number	Diff line change
`@@ -191,7 +191,7 @@ internal static void Shutdown()`
`191`	`191`	`Instance.started = false;`
`192`	`192`	`}`
`193`	`193`
`194`		`- internal nint DisposeAll()`
	`194`	`+ internal nint DisposeAll(bool disposeObj = true, bool disposeDerived = true, bool disposeBuffer = true)`
`195`	`195`	`{`
`196`	`196`	`if (_objQueue.IsEmpty && _derivedQueue.IsEmpty && _bufferQueue.IsEmpty)`
`197`	`197`	`return 0;`
`@@ -216,7 +216,7 @@ internal nint DisposeAll()`
`216`	`216`
`217`	`217`	`try`
`218`	`218`	`{`
`219`		`- while (!_objQueue.IsEmpty)`
	`219`	`+ if (disposeObj) while (!_objQueue.IsEmpty)`
`220`	`220`	`{`
`221`	`221`	`if (!_objQueue.TryDequeue(out var obj))`
`222`	`222`	`continue;`
`@@ -240,7 +240,7 @@ internal nint DisposeAll()`
`240`	`240`	`}`
`241`	`241`	`}`
`242`	`242`
`243`		`- while (!_derivedQueue.IsEmpty)`
	`243`	`+ if (disposeDerived) while (!_derivedQueue.IsEmpty)`
`244`	`244`	`{`
`245`	`245`	`if (!_derivedQueue.TryDequeue(out var derived))`
`246`	`246`	`continue;`
`@@ -258,7 +258,7 @@ internal nint DisposeAll()`
`258`	`258`	`collected++;`
`259`	`259`	`}`
`260`	`260`
`261`		`- while (!_bufferQueue.IsEmpty)`
	`261`	`+ if (disposeBuffer) while (!_bufferQueue.IsEmpty)`
`262`	`262`	`{`
`263`	`263`	`if (!_bufferQueue.TryDequeue(out var buffer))`
`264`	`264`	`continue;`