Fixed reloading .NET 6 add-in with ExplicitRegistration

[Sergey-Vlasov]

Added cleanup for allocated delegate handles.

[govert]

Might it be cleaner to add the delegateHandles.Add call into RegisterXlMethod instead?

[Sergey-Vlasov]

These handles are allocated inside XlMethodInfo.ConvertToXlMethodInfos, so delegateHandles.Add is tied to ConvertToXlMethodInfos, not to registration.

But I don’t want to add it to ConvertToXlMethodInfos either, because actual cleanup occurs in UnregisterMethods() and implies dependency on prior registration.

The current solution is in between. But also not ideal because of duplication…

[govert]

I might be misunderstanding something. It looks like, with your changes, all the paths that eventually call RegisterXlMethod will also store the GCHandle. I suppose I introduced the bug when adding some extra paths and not having those GCHandles adde too.

But the way I see it, inside RegisterXlMethod we are already storing some info into the other collections that XlRegistration keeps like registeredMethods (which is also part of the cleanup). So also keeping track of the delegate GCHandle there seems to make sense - it is the one method where all the registration mechanisms end up. Anyway, it seems equivalent but without the duplication, but I might be wrong.

Can you explain why this bug was causing the error when re-loading the add-in? How does that lead to the reload getting the DnaLibrary.CurrentLibrary wrong?

[Sergey-Vlasov]

Leaked handles prevented successful AssemblyLoadContext unloading (with all dlls) and when several dll copies are present in memory all things go wrong.

[Sergey-Vlasov]

Yes, currently we call RegisterXlMethod after each 'ConvertToXlMethodInfos' and storing GCHandle inside RegisterXlMethod will work.

But I imagine changes when 'ConvertToXlMethodInfos' may not be followed by a call to RegisterXlMethod. In this case will get a new leak.

Nevertheless, I'm fine to move storing GCHandle inside RegisterXlMethod.

[govert]

Leaked handles prevented successful AssemblyLoadContext unloading (with all dlls) and when several dll copies are present in memory all things go wrong.

Suppose we can't unload the ALC (I see we try to GC a few times, then give up). But we continue to reload anyway, so we make a new ALC (which has the same name - I don't know if that matters). Then I expect the new ALC to give isolation that works, at least for the ExcelDna assemblies. If not, I wonder if we're missing an ALC-aware load somewhere.

I'm not so worried about it, just curious to know why it breaks in this way. I do expect to have many scenarios where we don't currently unload successfully.

[Sergey-Vlasov]

Suppose we can't unload the ALC (I see we try to GC a few times, then give up). But we continue to reload anyway, so we make a new ALC (which has the same name - I don't know if that matters). Then I expect the new ALC to give isolation that works, at least for the ExcelDna assemblies. If not, I wonder if we're missing an ALC-aware load somewhere.

I'm not so worried about it, just curious to know why it breaks in this way. I do expect to have many scenarios where we don't currently unload successfully.

I can investigate that. And this bug provides a good repro case.

[govert]

I can investigate that. And this bug provides a good repro case.

Yes, I think that would be good. Maybe you can add a bit of logging along the way, e.g. it would be useful to know if the ALC does not get unloaded. Depending, of course, on a way to see the logging messages . . . :-)