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 clearedtp_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 callShutdown. 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();booleverythingSeemsCollected=TryCollectingGarbage(MaxCollectRetriesOnShutdown,forceBreakLoops:true);// this decreases the reference count

The tp_clear method:

publicstaticinttp_clear(BorrowedReferenceob){varweakrefs=Runtime.PyObject_GetWeakRefList(ob);if(weakrefs!=null){Runtime.PyObject_ClearWeakRefs(ob);}if(TryFreeGCHandle(ob))// what NullGCHandles tries to do after the fact{IntPtraddr=ob.DangerousGetAddress();booldeleted=CLRObject.reflectedObjects.Remove(addr);// the normal removal from hashsetDebug.Assert(deleted);}intbaseClearResult=BaseUnmanagedClear(ob);if(baseClearResult!=0){returnbaseClearResult;}ClearObjectDict(ob);return0;}

tp_clear should be responsible for freeing the GC handle and removing the address fromCLRObject.reflectedObjects what theforceBreakLoops tries to do separatedly.

for(inti=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)returntrue;}elseif(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 inCLRObject.reflectedObjects) to zero.
2. The reference count to all addresses inCLRObject.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 linepyCollected += 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(outvarobj))continue;if(obj.RuntimeRun!=run){HandleFinalizationException(obj.PyObj,newRuntimeShutdownException(obj.PyObj));continue;}IntPtrcopyForException=obj.PyObj;Runtime.XDecref(StolenReference.Take(refobj.PyObj));collected++;try{Runtime.CheckExceptionOccurred();}catch(Exceptione){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 aftertp_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, essentiallylist[int] whereint is the value ofGCHandle). That way we can have a variant ofTryCollectGarbage that does not refer toreflectedObjects and instead releasesGCHandle from that list.

Ideally, aftertp_clear is disabled we should also stop putting new objects intoreflectedObjects. Then theTryCollectGarbage won't need specialbool 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 theforceBreakLoop 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 theforceBreakLoop part could be removed completely in my change proposal because by the time my change gets to it, thereflectedObjects list is already empty. And I cannot really imagine that while running these lines anything is added to thereflectedObjects:

But I was not entirely sure whether or not there is a scenario in which objects still remain inreflectedObjects 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 theforceBreakLoop part could be removed completely. The first loop of theFinalizer.DisposeAll runs before the removal of thetp_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?

TheforceBreakLoop exists because you can have a .NET objectO that has aPyObject field pointing to Python objectP which somehow points back toO. 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 theO-P loop is only referenced by one of the reflected types. So whenRemoveTypes is called,O andP become garbage and might need to be collected. That constitutes the necessity to break loops even afterRemoveTypes.

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 theforceBreakLoop (beforeRemoveTypes) causes theLoopReference instance to be finalized at the nextGC.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 afterRemoveTypes. I tried some additional code variations and debugged through them but could not figure out how to produce a respective example.

usingSystem;namespaceTemp;publicclassLoopReference{privatereadonlyobject_P;publicLoopReference(objectp){_P=p;}~LoopReference(){if(_PisIDisposablep)p.Dispose();}}

PythonEngine.Initialize();PyDictlocals=newPyDict();PythonEngine.Exec(@"import clrclr.AddReference(r'[the path to the DLL containing LoopReference]')from Temp import LoopReferenceclass P(object):    def set_o(self, o):        self.o = op = 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 toreflectedObjects 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:
TheCollectOnShutdown test fails which I mentionedabove. The error messageGarbage 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