@amos402 can you take a look at this one. How does it affectFinalizer?

The termrun is a bit generic, and theprivate values should be prefixed by an underscore (I know that this is not everywhere the case, I'll try to watch out for it on future PRs). Apart from this I think this is an elegant solution, although I'm not quite sure whether the interlocked read is really required. Can newPyObjects be created whileInitialize is running?

@filmor the finalizer, that callsRuntime.GetRun() will run from .NET finalizer thread, which is always a dedicated thread. So at least the finalizer thread and the thread, that runsRuntime.Initialize must somehow synchronize.

@filmor , BTW, this is not a fix. I just wanted to see if the PR would have the problem stand out in a CI run (which it does, but only a few tests fail; the effect would be much more dramatic, ifFinalizer would not simply eat all exceptions, that happens on a scale).

But can't this be handled by simply locking a mutex in the thread and Initialize?

But can't this be handled by simply locking a mutex in the thread and Initialize?

That would have basically the same effect at slightly higher cost.

We need input here to decide what to do with the problem.@amos402@benoithudson@Martin-Molinero

After giving this some thought, I feel we might need to explicitly callGC.Collect andGC.WaitForPendingFinalizers inPythonEngine.Shutdown so that all dead .NET objects holding references to Python objects would release them to letPy_Finalize collect as much Python memory as possible before the run/generation ends.

Huh? The current implementation introduces cost (atomic read) for every single object created, while the lock would only need to be taken during the finalization and in Initialize.

@filmor you can't really forego some sort of synchronization on everyPyObject created, as without synchronization it might read stale value. Seehttps://shipilev.net/blog/2014/jmm-pragmatics/

On x86 and AMD64 all reads are atomic AFAIK, so this only affects ARM.

@filmor actually, in x86 builds this change would have performance impact, as extra steps are needed to guaranteeInt64 read atomicity. I need to changelong run toint run and useVolatile.Read instead ofInterlocked.Read.

This feels like something that should on in a Debug build but not in Release.

For cleanup you basically need:

            System.GC.Collect();            using (Py.GIL())            {                dynamic gc = Py.Import("gc");                gc.collect();            }            System.GC.Collect();

The first C# collect cleans garbage on the C# side. That might release some references into Python.

The Python collect cleans garbage on the Python side, which might release some references into C#. It also releases the Python side of handles that the C# collect released.

The second C# collect releases the handles the Python collect released. Not strictly necessary given that we're shutting down C# anyway.

Just to clarify, this is not talking about CLR domain unload, justPythonEngine.Shutdown.

This is whatPy_Finalize doc says:

Ideally, this frees all memory allocated by the Python interpreter.
Memory tied up in circular references between objects is not freed. Some memory allocated by extension modules may not be freed.

I think we should strive for "ideally". Unless we force collection of dead .NETPyObjects, Python will treat the objects that are referenced by them as "tied up", when they actually are not.

Alternatively, we could document this, and suggest users to callGC.Collect andWaitForPendingFinalizers manually before callingPythonEngine.Shutdown to avoid forcing this on everyone.

@filmor OK, turns outVolatile is not enough, and one needsInterlocked.Volatile does not guarantee "freshness", e.g. it is only eventually consistent.

I think garbage collecting on PythonEngine.Shutdown makes loads of sense; otherwise you irreparably leak memory and you slow down Python gc if you restart Python later.

Python automatically does a gc pass during Py_Finalize so you might not need to run Python gc during Shutdown. But I don't think it hurts.

Oh, just realized you might be reading something different than intended:

This feels like something that should on in a Debug build but not in Release.

I was referring to this PR, asserting that PyObject is only finalized in the proper run. Garbage collecting seems like it should always be done.

TBH, I failed to find the relevant documentation, but from a quick ducking around my understanding is thatPy_Finalize will attempt to destroy all objects, allocated by Python (e.g. instances ofC typePyObject).

Py_Finalize only destroy the garbages.

That means, that any .NET objects, that own handles to Python objects (e.g.PyObject orPythonException) should not try to deallocate those handles afterPy_Finalize has been called, even if Python runtime is later initialized again for a newrun.

It is. Thus I always trend to not using the pythonnet's features in the internal.

In this change I add run tracking toRuntime andPyObject, and force-crashPyObject.Dispose instead of callingXDecref when they do not match. We need to determine what should the proper behavior be in that scenario.

As@filmor mentioned, I'm not like to increase the overhead here too, maybe just enable it on debug mode.

I believe#958 also suffers from this issue.

I think it's not related to#958, since the domain may changed, it will not call the dispose in another runime, these objects already leak by theFinalizer.
Also#958 can release the the object which created by previous runtime as long as it didn't leaked byFinalizer.

😂Let me guess, you encounter the crash when you testing, and it seems decrease reference count beyond the object, but it it may just the reference count error of PyObject.
At this point, you may turn on TRACE_ALLOC and add a listener toFinalizer.IncorrectRefCntResolver, check the tracebacks of PyObject creation and memo it , rerun the test again with some tracks for this object to figure it out.

@amos402 no, "decrease reference count" is not the what this is about (or at least I did not check, may be related). This is about the following scenario:

PythonEngine.Initialize();varnum=42.ToPyObject();PythonEngine.Shutdown();// <- "run" 1 endsPythonEngine.Initialize();// <- "run" 2 startsnum=null;GC.Collect();GC.WaitForPendingFinalizers();// <- this will put former `num` into Finalizer queueFinalizer.Instance.Collect(forceDispose:true);// ^- this will call num.Dispose, which will call XDecref on `num.Handle`,// but Python interpreter from "run" 1 is long gone, so it will corrupt memory instead.

In the example above no class checks, that the original interpreter is gone.

If the reference count turns to 0, it will call the tp_dealloc, so the matter is determine the type slots, not the place it call XDecref ,#958 should be able to handle it.
Add some simple tests in#958 for this.https://github.com/pythonnet/pythonnet/pull/958/files/183f9d822c17b1cba7f3538b8d4394b7ef1cae44..9d57a82732053f408b8d115d1035177d5c077bef#diff-0731db125845d153331b229612722c5c

@amos402 the tests you've added only check that the object is collected. But the problem is collecting it might corrupt memory. To see the effect, you might need to run it in a loop, and also ensure no exception is generated byFinalizer.ErrorHandler.

I updated the tests for running multi times and passed. I test it for 1000 times locally, but it will take too much time due to calling the GC, some I reduce it to 10 on commit.
Dispose cross should be allow in#958, if it cause corrupted memory, that will be a bug.

Played a bit with the test Amos added tosoft-shutdown branch, and it seem to be working correctly no matter which object survives until the next run/generation of interpreter.

@amos402 since it appears to be safe to call decref on objects from previous generation, should not we keep .NET objects that need finalization alive until the runtime is up again?

Reopening this pull request for 3.0 milestone. I discovered a trivial example that (at least in Python 3.10, maybe others too) corrupts Python heap. Being unsure if this is a bug in CPython, I openedan issue in CPython bug tracker.

A trivial repro using using Python.NET code base and Python 3.10 AMD64 (debug build) on Windows:

Runtime.Py_Initialize();BorrowedReferencebuiltins=Runtime.PyEval_GetBuiltins();BorrowedReferenceiter=Runtime.PyDict_GetItemString(builtins,"iter");varownedIter=newNewReference(iter);// this basically does IncRefRuntime.Py_Finalize();Runtime.Py_Initialize();ownedIter.Dispose();// this basically does DecRefRuntime.Py_Finalize();// <- this blows up in PyGC_Collect -> validate_list

@amos402 if you have time, I would like to hear your thoughts on this.

@filmor with a workaround for Mono hanging in one of theGC calls, all tests pass.

Can you review this pull request?