KSP-ModularManagement / KSPe

Extensions and utilities for Kerbal Space Program
http://ksp.lisias.net/add-ons/KSPAPIExtensions
Other
11 stars 5 forks source link

"System.NotSupportedException: The invoked member is not supported in a dynamic module." being triggered by KSPe's reflection routines on the wild #59

Open Lisias opened 11 months ago

Lisias commented 11 months ago

Task:

Write yet another Standard Error Handler for this System.NotSupportedException thingy, aborting KSP the same by pinpointing a Q&A Page where the problem is described and possible work arounds suggested.

— —

Well… TL;DR:

https://forum.kerbalspaceprogram.com/topic/179030-ksp-130-tweakscale-under-lisias-management-2474-2023-1007/?do=findComment&comment=4328731

And this not a novelty on KSP:

https://forum.kerbalspaceprogram.com/search/?q=%22System.NotSupportedException%22&quick=1&updated_after=any&sortby=newest&search_and_or=or

And the fix/work-around appears to be simple enough:

https://github.com/blizzy78/ksp_toolbar/pull/39

It's important to note that the last link is pinpointing something that happened in 2016, way before KSP 1.4, Harmony and KSPCF. It's almost sure something on Unity itself - or something "interesting" on KSP that lingers way for more time than I thought?

Lisias commented 11 months ago

Oukey, the plot thickens. It's not Unity.

It was determined that KopernicusExpansion is deploying a thing called ReSharp.dll, used to emit Assemblies programatically. This thing is present on some logs on my archive from previous tickets since Nov 2021 at least, but only recently (Feb 2023) started to bite.

The original ReSharp is kinda abandoned since 2009, I found repositories on SourceForge and on the late Google Code but the one I found on KEX is the Aqla fork.

On a side note, Forum rules demands that the source code of every code deployed on KSP to be available. KEX guys should pinpoing the Aqla's RunSharp source code on their documentation.

Lisias commented 11 months ago

Ha!! That's precious!!!

This is the KSP.log's line where the problem happens:

[LOG 16:26:59.210] [TweakScale] ERROR: System.NotSupportedException: The invoked member is not supported in a dynamic module.
  at System.Reflection.Emit.AssemblyBuilder.get_Location () [0x00006] in <9577ac7a62ef43179789031239ba8798>:0
  at KSPCommunityFixes.Modding.ReflectionTypeLoadExceptionHandler+FailedAssembly..ctor (System.Reflection.Assembly assembly) [0x000b8] in <84a4e11594ad4c4cb519f08a8e322246>:0
  at KSPCommunityFixes.Modding.ReflectionTypeLoadExceptionHandler.LogReflectionTypeLoadExceptionInfo (System.Reflection.Assembly assembly) [0x00021] in <84a4e11594ad4c4cb519f08a8e322246>:0
  at KSPCommunityFixes.Modding.ReflectionTypeLoadExceptionHandler.Assembly_GetTypes_Prefix (System.Reflection.Assembly __instance, System.Type[]& __result) [0x0000c] in <84a4e11594ad4c4cb519f08a8e322246>:0
  at (wrapper dynamic-method) System.Reflection.Assembly.System.Reflection.Assembly.GetTypes_Patch1(System.Reflection.Assembly)
  at KSPe.Util.SystemTools+Type+Find.ByQualifiedName (System.String qn) [0x00039] in <58fb44557e3d487fa13c42bddbc423e1>:0
  at TweakScale.Startup.Start () [0x000a3] in <8e463f0c7a754587854563c7c6517452>:0  at error:0

And this is the interesting part:

System.Reflection.Assembly.System.Reflection.Assembly.GetTypes_Patch1(System.Reflection.Assembly)
  at KSPe.Util.SystemTools+Type+Find.ByQualifiedName (System.String qn) [0x00039] in <58fb44557e3d487fa13c42bddbc423e1>:0

There's no GetTypes_Patch1 on the type System.Reflection.Assembly.System.Reflection.Assembly!! So the bug is on the code being injected by someone else.

THIS IS BEYOUND ME, I don't have how to fix it!!!!

Lisias commented 11 months ago

Apparently it's KSPCF.

The Exception is being thrown inside a method called GetTypes_Patch1 on System.Reflection.Assembly.System.Reflection.Assembly that does not exists for me.

The "RunSharp.dll" is, indeed, a tool for making easier to use System.Reflection.Emit, that it's a tool to generate code at runtime. So someone, somewhere, is generating Dynamic Assemblies using this stunt.

But what's causing the problem is something that was injected on System.Reflection.Assembly.System.Reflection.Assembly using something like Harmony or similar.

So I remembered that KSPCF once published a fix/work-around/whatever for the infamous Assembly Loader/Resolver, and I'm pretty sure that code would need to handle the .Location property (that it's known to trigger the Unsupported Exception on dynamic assemblies).

Checking KSPCF source code, I found this line:

if (methodName.StartsWith("<SetupFSM>") && !methodName.Contains("_Patch"))

what suggests that KSPCF should be, indeed, be involved somehow on this mess.

I also found that KSPCF is, in fact, directly accessing the GetTypes method (see here, and here). So we have evidences enough to drop them this hot potato.

https://forum.kerbalspaceprogram.com/topic/179030-ksp-130-tweakscale-under-lisias-management-2474-2023-1007/?do=findComment&comment=4328822

Lisias commented 11 months ago

This is not on my shoulders, there's absolutely nothing I can do about.

gotmachine commented 11 months ago

Neither the root cause of the user issue (missing dependency, or incompatible assembly) or its consequences on TweakScale are due to KSPCF, please read my answer on the KSPCF thread : https://forum.kerbalspaceprogram.com/topic/204002-18-112-kspcommunityfixes-bugfixes-and-qol-tweaks/?do=findComment&comment=4328959

One reason why I decided to handle a ReflectionTypeLoadException when any stock or third party code is calling Assembly.GetTypes() from KSPCF is because many plugins, including TweakScale, are forgetting to try/catch that exception to avoid being affected by a third-party assembly failing to load.

The other reason is to visually provide as much information as possible to end-users about the plugin that failed to load.

This being said, there is indeed an oversight in that code causing it to in turn throw an NotSupportedException when a dynamic assembly is present, and I will push a fix for that ASAP. But note that the end result of that bug is the same as in a stock install. Although I will concede that a different exception is thrown, which might cause a different behavior if one is specifically catching a ReflectionTypeLoadException, but TweakScale/KSPe does neither, so there is no difference.

In any case, I would highly suggest that you add exception handling to all your GetTypes() calls (you could do that as an extension method). Strangely, you actually have that handling in one place : https://github.com/net-lisias-ksp/KSPe/blob/f180aa1640dde37b097e6a3fa135e2d52d4ac5c9/Source/KSPAPIExtensions/zzVersionChecker.cs#L208-L215

But not in others : https://github.com/search?q=repo%3Anet-lisias-ksp%2FKSPe%20gettypes&type=code

Lisias commented 11 months ago

Neither the root cause of the user issue (missing dependency, or incompatible assembly) or its consequences on TweakScale are due to KSPCF,

You are right. I came to the same conclusion, but didn't updated this issue. Sorry for that.

I will update with the information I had posted on Forum as time allows

Lisias commented 11 months ago

Oukey, back to basics:

From https://learn.microsoft.com/en-us/dotnet/api/system.reflection.assembly.gettypes?view=net-3.5 , we get:

Exceptions ReflectionTypeLoadException The assembly contains one or more types that cannot be loaded. The array returned by the Types property of this exception contains a Type object for each type that was loaded and null for each type that could not be loaded, while the LoaderExceptions property contains an exception for each type that could not be loaded.

So the official documentation does not tells anything about the System.NotSupportedException. Whoever did the unfortunate implementation of that Get_Types implementation broke the specification. Point.

Writing shitty code on my side to workaround shitty code from someone else is not an option, so besides being grateful for the suggestion, I will not do it. Whoever is calling KSPe for listing the currently loaded Assemblies wants a list of the currently loaded assemblies For a Reason™, and returning back an empty list will lead to further errors on the stack - what will only make diagnosing more difficult. I do not sweep dirty under my carpet.

The solution will be, indeed, to write yet a new Standard Error Handler for this System.NotSupportedException thingy, aborting KSP the same by pinpointing a Q&A Page where the problem is described and possible work arounds suggested.

Lisias commented 9 months ago

I will tackle this one on 2.5.4.0 (or earlier, if I issue another bug fix before it)