Track Runtime run number #1074
Track Runtime run number #1074
Conversation
|
@amos402 can you take a look at this one. How does it affect |
|
The term |
|
@filmor the finalizer, that calls |
|
@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, if |
|
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 call |
|
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 every 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 guarantee |
|
This feels like something that should on in a Debug build but not in Release. For cleanup you basically need: 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, just This is what Py_Finalize doc says:
I think we should strive for "ideally". Unless we force collection of dead .NET Alternatively, we could document this, and suggest users to call |
|
@filmor OK, turns out |
|
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:
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. |
It is. Thus I always trend to not using the pythonnet's features in the internal. pythonnet/src/runtime/finalizer.cs Lines 104 to 109 in 925c166 Just as I commented. For safe, they would be leak. Dispose will not be called on another runtime.FIX: If the domain didn't reload, it may be called on another runtime, but #958 should enable to handle it.
As @filmor mentioned, I'm not like to increase the overhead here too, maybe just enable it on debug mode.
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 the |
|
😂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. |
|
@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();
var num = 42.ToPyObject();
PythonEngine.Shutdown(); // <- "run" 1 ends
PythonEngine.Initialize(); // <- "run" 2 starts
num = null;
GC.Collect();
GC.WaitForPendingFinalizers(); // <- this will put former `num` into Finalizer queue
Finalizer.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. |
|
@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 by |
|
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. |
|
Played a bit with the test Amos added to @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 opened an 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();
BorrowedReference builtins = Runtime.PyEval_GetBuiltins();
BorrowedReference iter = Runtime.PyDict_GetItemString(builtins, "iter");
var ownedIter = new NewReference(iter); // this basically does IncRef
Runtime.Py_Finalize();
Runtime.Py_Initialize();
ownedIter.Dispose(); // this basically does DecRef
Runtime.Py_Finalize(); // <- this blows up in PyGC_Collect -> validate_list@amos402 if you have time, I would like to hear your thoughts on this. |
ffaa254 to
8658322
Compare
… the same run they were created in.
…Python runtime is shut down
… on shutdown when loaded from Python
|
@filmor with a workaround for Mono hanging in one of the Can you review this pull request? |
…h other PythonNET properties
Assert, that
PyObjects are only disposed in the same run they were created in.What does this implement/fix? Explain your changes.
TBH, I failed to find the relevant documentation, but from a quick ducking around my understanding is that
Py_Finalizewill attempt to destroy all objects, allocated by Python (e.g. instances of C typePyObject).That means, that any .NET objects, that own handles to Python objects (e.g.
PyObjectorPythonException) should not try to deallocate those handles afterPy_Finalizehas been called, even if Python runtime is later initialized again for a new run.Right now
PyObject.Disposeonly checks forPy_IsInitialized, without giving any regard to which runtime run does it belong to versus which run is it now.In this change I add run tracking to
RuntimeandPyObject, and force-crashPyObject.Disposeinstead of callingXDecrefwhen they do not match. We need to determine what should the proper behavior be in that scenario.I believe #958 also suffers from this issue.
Does this close any currently open issues?
Related to #1073 , #958