fix: catch collectible access non-collectible exception

[gnattu]

Changes

Catch the NotSupportedException caused by plugins directly using the server's XML serializer and throw a new one with informative messages, notifying the plugin developer that directly using this serializer is not a supported use case.

This is an alternative to #11254 to retain the in-process restart functionality.

This change could have a long-term impact, meaning that our plugins need to use their own XML serializers from now on, and this change may not be limited to 10.9, but also in later versions.

Issues

Fixes #11251

[mark-monteiro]

Instead of this, it might be possible to provide an alternate ServiceProvider when constructing the IPlugin instances in the plugin manager. A custom serviceprovider that wraps the standard one, but throws this exception if an attempt is made to resolve the xml serializer. If that works I think it would be a cleaner solution.

[gnattu]

Instead of this, it might be possible to provide an alternate ServiceProvider when constructing the IPlugin instances in the plugin manager. A custom serviceprovider that wraps the standard one, but throws this exception if an attempt is made to resolve the xml serializer. If that works I think it would be a cleaner solution.

How would that work? The MyXmlSerializer instance is directly passed into the plugin's constructor

[cvium]

Do we have a minimal repro plugin?

[gnattu]

Do we have a minimal repro plugin?

Not exactly minimal. I tried to create one before, but the minimal version didn't have this problem for some reason. I ended up reproducing the issue with this repository: jumoog/intro-skipper@5d74330

At this specific commit, I can reproduce the issue by clicking "analyze intro" in the scheduled tasks.

[jumoog]

https://github.com/jumoog/intro-skipper/blob/5d743302e41c88f0e8ac85cee88142ca1a235aee/ConfusedPolarBear.Plugin.IntroSkipper/Plugin.cs#L218

That throws System.NotSupportedException: A non-collectible assembly may not reference a collectible assembly.

[mark-monteiro]

How would that work? The MyXmlSerializer instance is directly passed into the plugin's constructor

I was thinking the plugin would need to construct it's own instance of MyXmlSerializer to use. I think I may have also misunderstood what needs to be done here to fix this though so perhaps my suggestion would not fix anything.

I am travelling at the moment and don't have regular access to a computer, so feel free to continue on this without additional feedback/approval from me.

I was mainly intrigued about looking into this because this same error was what blocked me when I originally tried to implement plugin unloading. Back then, this was a bug in the dotnet runtime (also this and this related bug) that caused this error to pop up any time XmlSerializer was used in a collectible assembly load context. However, that bug was fixed way back in .NET 6, which allowed someone else to continue on and finish the implementation. So I'm surprised to now see it pop up again.

Perhaps reading through those linked runtime issues might provide some clues on how to get this working. I also came across this documentation article on how to analyze assembly loading issues: https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/collect-details. That all seems pretty complicated/confusing though, so if this quick fix also solves the issue then maybe just go for it 🤷

[gnattu]

I was mainly intrigued about looking into this because this same error was what blocked me when I originally tried to implement plugin unloading. Back then, this was a bug in the dotnet runtime (also this and this related bug) that caused this error to pop up any time XmlSerializer was used in a collectible assembly load context. However, that bug was fixed way back in .NET 6, which allowed someone else to continue on and finish the implementation. So I'm surprised to now see it pop up again.

It shows the same exception but caused by different reasons. The bugs you linked are for all XmlSerializer instances created in the plugin, but this instance is created in the server and passed into the constructor. In your bugs, the plugin cannot even be loaded if it wants to initialize an XmlSerializer. For our current situation, passing this instance is fine; it won't block the load of the plugin. Using this serializer from the server side is also fine; the server is using it for plugin config files. However, accessing this instance and using its methods to try to serialize/deserialize in the plugin itself is not supported because it will cause runtime errors like NullReferenceException or even crashes, and that's why you are seeing this Exception. The serializer will generate some dynamic assemblies that is not collectible and hence cannot be used in the plugin.

Perhaps reading through those linked runtime issues might provide some clues on how to get this working. I also came across this documentation article on how to analyze assembly loading issues: https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/collect-details. That all seems pretty complicated/confusing though, so if this quick fix also solves the issue then maybe just go for it 🤷

This should not work by design. The original design of in-process reloading overlooked the problems this approach may cause. This is fine, because even Microsoft overlooked this and have to patch it later in dotnet 7. The way it worked is not supposed to work, and the fact that it actually worked is a bug and should be fixed. We should not try to "get this working" by "analyzing assembly loading issues" because it is not an issue, but a feature limitation. If we really want the plugin to share the serializer with the server, we have to do much more work to replace the XmlSerializer with something that has less runtime magic, such as the DataContractSerializer. However, this obviously requires much more work and will introduce a lot of breaking changes, which is not suitable for 10.9.

[jumoog]

In my opinion, this is also important for the 10.9 release.

[jellyfin-bot]

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.