Alright. At least the embedding tests went through. I am going to take another look.

The original change broke the shutdown flow for derived objects. The (hopefully) final proposition from me remains the same, only restricting the early collection run to the managed objects which would, as described above, run into the problem of the cleared tp_clear slot at a later point.

@filmor @lostmsu Could you please trigger the workflows/pipelines again?

This feels like a band-aid rather than proper fix. Theoretically invoking GC should not leave anything in a broken state, but types derived across .NET/Python boundaries are tricky. We should come up with a correct way to finalize them rather than adding such a band-aid, IMHO.

It is possible your scenario keeps invalid references. E.g. Python or .NET hold live references to each other's objects at the point when you call Shutdown. You need to narrow down the scenario to a short test case that would reliably crash, then we can see if Python.NET needs to fix that and how or provide a better error message.

@lostmsu To make sure we are on the same page and in risk of repeating myself, I will copy some code snippets of the current master.

The shutdown method:

            NullGCHandles(ExtensionType.loadedExtensions);
            ClassManager.RemoveClasses();
            TypeManager.RemoveTypes(); // this removes tp_clear slot
            _typesInitialized = false;

            ...

            PyGC_Collect();
            bool everythingSeemsCollected = TryCollectingGarbage(MaxCollectRetriesOnShutdown,
                                                                 forceBreakLoops: true); // this decreases the reference count

The tp_clear method:

        public static int tp_clear(BorrowedReference ob)
        {
            var weakrefs = Runtime.PyObject_GetWeakRefList(ob);
            if (weakrefs != null)
            {
                Runtime.PyObject_ClearWeakRefs(ob);
            }

            if (TryFreeGCHandle(ob)) // what NullGCHandles tries to do after the fact
            {
                IntPtr addr = ob.DangerousGetAddress();
                bool deleted = CLRObject.reflectedObjects.Remove(addr); // the normal removal from hashset
                Debug.Assert(deleted);
            }

            int baseClearResult = BaseUnmanagedClear(ob);
            if (baseClearResult != 0)
            {
                return baseClearResult;
            }

            ClearObjectDict(ob);
            return 0;
        }

tp_clear should be responsible for freeing the GC handle and removing the address from CLRObject.reflectedObjects what the forceBreakLoops tries to do separatedly.

                for (int i = 0; i < 2; i++)
                {
                    GC.Collect();
                    GC.WaitForPendingFinalizers();
                    pyCollected += PyGC_Collect();
                    pyCollected += Finalizer.Instance.DisposeAll();
                }
                if (Volatile.Read(ref _collected) == 0 && pyCollected == 0)
                {
                    if (attempt + 1 == runs) return true;
                }
                else if (forceBreakLoops)
                {
                    NullGCHandles(CLRObject.reflectedObjects);
                    CLRObject.reflectedObjects.Clear();
                }

There are two crucial points which I hope you see are wrong in the current shutdown flow:

1. tp_clear is not invoked after decreasing the reference count of a respective object (referenced in CLRObject.reflectedObjects) to zero.
2. The reference count to all addresses in CLRObject.reflectedObjects is zero when calling NullGCHandles(CLRObject.reflectedObjects);, i.e. handling the addresses in that state.

This can be seen by just debugging and stepping into the code, regardless if an exception is thrown or not. Hence, the state is already broken after the line pyCollected += Finalizer.Instance.DisposeAll();.

Is this not enough that a fix is necessary?
An error message does not help because nothing can be done really to handle it. If you extend Python and unload pythonnet, you cannot use C# exception types anymore to catch them in the Python domain. This is an unhandled exception that crashes the Python process.

This feels like a band-aid rather than proper fix.

Is this a feeling because of function name semantics which could be dissolved by refactoring the solution?

Effectively, my changes only run this:

                    while (!_objQueue.IsEmpty)
                    {
                        if (!_objQueue.TryDequeue(out var obj))
                            continue;

                        if (obj.RuntimeRun != run)
                        {
                            HandleFinalizationException(obj.PyObj, new RuntimeShutdownException(obj.PyObj));
                            continue;
                        }

                        IntPtr copyForException = obj.PyObj;
                        Runtime.XDecref(StolenReference.Take(ref obj.PyObj));
                        collected++;
                        try
                        {
                            Runtime.CheckExceptionOccurred();
                        }
                        catch (Exception e)
                        {
                            HandleFinalizationException(obj.PyObj, e);
                        }
                    }

Before this:

            NullGCHandles(ExtensionType.loadedExtensions);
            ClassManager.RemoveClasses();
            TypeManager.RemoveTypes();
            _typesInitialized = false;

            MetaType.Release();
            PyCLRMetaType.Dispose();
            PyCLRMetaType = null!;

            Exceptions.Shutdown();
            PythonEngine.InteropConfiguration.Dispose();
            DisposeLazyObject(clrInterop);
            DisposeLazyObject(inspect);
            DisposeLazyObject(hexCallable);
            PyObjectConversions.Reset();

An explanation is not enough, we need a minimal test (not necessarily written as a test function, could be a short script or .NET console app) that fails before the fix, and passes after, preferably with just a handful of allocated objects.

I am being so picky because without certainty about exactly what's going on I can not be sure that your fix does not just randomly happens to help your particular use case while breaking someone else's use case not yet covered by tests. And it introduces a little extra complexity too.

This is the most down grinded version of my case that I could accomplish: https://github.com/Frawak/pythonnet/tree/fix/1977-access-violation-gc-testcase/exception_scenario
Please read the README.

I hope that the exception is as reliable on other machines.

preferably with just a handful of allocated objects

The problem with that: The exception only occurs with some internal memory management from Python (my interpretation) which is hard to reproduce in general and I do not see how it can be done with a few simple objects. The process has to be triggered by its inner logic to access the memory while pythonnet is shutting down.

I also had to add the other open-source library because while using that, the exception occurred in specific cases. But there is nothing wrong with the library and its types. They are pretty simple and no IDisposables or the like.
Trying to magically trigger this exception with default .NET library types is like gambling in the lottery.

And as I said: It is not about the surrounding conditions when the exception is thrown. They are not the cause. The cause can be seen with any script using .NET objects from Python and debugging into the shutdown method. It is just lucky that the exception is rare.

@lostmsu Are you going to look at the code example? Does the exception get thrown when you run it?

It's a process. Work with me.

@Frawak I took a look at the example, just want to see if I understood it correctly: it does not appear to have either Python classes derived from .NET classes, nor .NET classes derived from Python classes? E.g. all it does is imports .NET lib, instantiates a few classes from there as temporary variables, and that's it?

P.S. sorry about late response, we are unpaid maintainers here, doing it at our leisure.

I took a look at the example, just want to see if I understood it correctly: it does not appear to have either Python classes derived from .NET classes, nor .NET classes derived from Python classes? E.g. all it does is imports .NET lib, instantiates a few classes from there as temporary variables, and that's it?

That is correct.

P.S. sorry about late response, we are unpaid maintainers here, doing it at our leisure.

No need to apologize. Of course I know that. But any information (like "maybe in x weeks") is better than no information. Otherwise, I don't know whether it has genuinely slipped your mind and you need a poke or you come around to it at your own time. In my experience, the former is more common.

@lostmsu could you please help take another look?

Just wanted to report that we were consistently running into this issue in our Windows CI tests, for a case where we were using pythonnet to load a proprietary .net file format parser (since it's proprietary, can't necessarily submit it as a test case).

We switched to point to this branch of pythonnet, and the issue has gone away.

So, hopefully that might provide another data point to address "I can not be sure that your fix does not just randomly happens to help your particular use case while breaking someone else's use case not yet covered by tests," this does fix a problem for us and doesn't seem to have introduced any new ones across our test matrix (Python 3.10, 3.11, 3.12 on Debian, and 3.10 on Windows).

Please do review and ensure this branch is properly tested, but we are eagerly looking forward to the release of this fix, or some other fix to this problem.

@lostmsu The current investigation here would be enough for me to accept the PR as is. That derived classes require special handling is not surprising, given how they are implemented.

@Frawak It would be great if you could put your test-case into a separate repository and keep it alive for a while. If you have the time to integrate it into our test-suite, even better :)

I still plan to take a look.

OK, I did a review, and IMHO, this fix will just leak .NET memory because the GC handles will never be released after .NET types are disabled in Python. I am not sure, but it also might mean that some critical finalizers won't run.

IMHO, what should be done is after tp_clear is disabled, we should replace it with a Python function that will grab the GC handle and put it somewhere the rest of the shutdown procedure can look (e.g. some global Python object, essentially list[int] where int is the value of GCHandle). That way we can have a variant of TryCollectGarbage that does not refer to reflectedObjects and instead releases GCHandle from that list.

Ideally, after tp_clear is disabled we should also stop putting new objects into reflectedObjects. Then the TryCollectGarbage won't need special bool telling it if the objects in this list are still valid.

I have a guess what you could mean but I am not following completely.

because the GC handles will never be released after .NET types are disabled in Python.

tp_clear manages that on its own:

That is the same thing what the forceBreakLoop part tries to do after the fact:

Ideally, after tp_clear is disabled we should also stop putting new objects into reflectedObjects. Then the TryCollectGarbage won't need special bool telling it if the objects in this list are still valid.

I would even suggest that the forceBreakLoop part could be removed completely in my change proposal because by the time my change gets to it, the reflectedObjects list is already empty. And I cannot really imagine that while running these lines anything is added to the reflectedObjects:

But I was not entirely sure whether or not there is a scenario in which objects still remain in reflectedObjects after running the first part of the finalizer:

Because in my scenario I do not use any derived types or anything exotic, only:

all it does is imports .NET lib, instantiates a few classes from there as temporary variables, and that's it

And so my changes made only the most minimalistic touch on the shutdown behavior so that the previous code parts still run if I am overlooking something. And if I am not, then that code just runs but has no effect anymore (which then is just a redundancy at its worst).

To break it down:
My gut feeling is that the forceBreakLoop part could be removed completely. The first loop of the Finalizer.DisposeAll runs before the removal of the tp_clear slot and the other loops after as my changes suggest.
But could you give a scenario in which .NET types are still in play and risk of not being finalized properly?

The forceBreakLoop exists because you can have a .NET object O that has a PyObject field pointing to Python object P which somehow points back to O. If there are only these two objects, and nothing else references them, in order to get them collected one needs to destroy one of the references.

It is possible that the O-P loop is only referenced by one of the reflected types. So when RemoveTypes is called, O and P become garbage and might need to be collected. That constitutes the necessity to break loops even after RemoveTypes.

I created the scenario of your first paragraph with the code snippets shown below and debugged into the code (with my changes).
I could observe that the forceBreakLoop (before RemoveTypes) causes the LoopReference instance to be finalized at the next GC.Collect(). Otherwise, the finalizer gets called when the program exits. So, I get the point behind it now.
But I do not comprehend the necessity of breaking loops after RemoveTypes. I tried some additional code variations and debugged through them but could not figure out how to produce a respective example.

using System;
namespace Temp;
public class LoopReference
{
    private readonly object _P;

    public LoopReference(object p)
    {
        _P = p;
    }

    ~LoopReference()
    {
        if (_P is IDisposable p)
            p.Dispose();
    }
}

PythonEngine.Initialize();

PyDict locals = new PyDict();

PythonEngine.Exec(@"
import clr
clr.AddReference(r'[the path to the DLL containing LoopReference]')
from Temp import LoopReference
class P(object):
    def set_o(self, o):
        self.o = o
p = P()
o = LoopReference(p)
", null, locals);

PythonEngine.Shutdown();

Ideally, after tp_clear is disabled we should also stop putting new objects into reflectedObjects.

Essentially, this is the biggest puzzle piece for me. How are new objects added? I do not see any follow-up code in the shutdown procedure that does that.

Maybe you could introduce a flag that makes adding an object to reflectedObjects throw after Shutdown? That would at least give us a stacktrace.

@filmor Like so (see last commit)?

Could you start the workflows again? I ran the tests locally.

In one of the workflow configurations, an obsolete test failed:

The failed assert is also before running the shutdown.

Can confirm this branch fixes my Access Violation Exception issue I've been getting. Any idea on when this can be released?

@lostmsu Anything else you would like to see?

Nice, thank you very much for your contribution @Frawak. I'll see this weekend if there is more to do before cutting a new release.

Not sure if it's related yet, but we have two crashes in CI.

The difference in the log of the two failed jobs compared to others is the following:
The CollectOnShutdown test fails which I mentioned above. The error message Garbage should contains the collected object indicates that the test fails before the shutdown is called, i.e. before any change.

Links to the log lines:
https://github.com/pythonnet/pythonnet/actions/runs/10274663048/job/28431871059#step:10:195
https://github.com/pythonnet/pythonnet/actions/runs/10274663048/job/28431871476#step:10:199