Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"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 this issue Oct 7, 2023 · 8 comments
Assignees
Labels
task Not a problem, but something I must do and should not forget about unrelated This is something on some other Add'On.
Milestone

Comments

@Lisias
Copy link
Collaborator

Lisias commented Oct 7, 2023

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:

blizzy78/ksp_toolbar#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 Lisias self-assigned this Oct 7, 2023
@Lisias Lisias added bug Something isn't working not-my-fault I'm innocent on this! :) labels Oct 7, 2023
@Lisias Lisias added this to the 2.5.3.4 milestone Oct 7, 2023
@Lisias
Copy link
Collaborator Author

Lisias commented Oct 7, 2023

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
Copy link
Collaborator Author

Lisias commented Oct 7, 2023

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
Copy link
Collaborator Author

Lisias commented Oct 7, 2023

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 Lisias added wontfix This will not be worked on unrelated This is something on some other Add'On. BLOCKED This task is currently blocked by something else and removed not-my-fault I'm innocent on this! :) labels Oct 7, 2023
@Lisias
Copy link
Collaborator Author

Lisias commented Oct 7, 2023

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

@gotmachine
Copy link

gotmachine commented Oct 8, 2023

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
Copy link
Collaborator Author

Lisias commented Oct 9, 2023

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
Copy link
Collaborator Author

Lisias commented Oct 9, 2023

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 Lisias added task Not a problem, but something I must do and should not forget about and removed bug Something isn't working wontfix This will not be worked on BLOCKED This task is currently blocked by something else labels Oct 9, 2023
@KSP-ModularManagement KSP-ModularManagement locked as resolved and limited conversation to collaborators Oct 9, 2023
@Lisias Lisias modified the milestones: 2.5.3.4, 2.5.4.0 Nov 17, 2023
@Lisias
Copy link
Collaborator Author

Lisias commented Nov 17, 2023

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
task Not a problem, but something I must do and should not forget about unrelated This is something on some other Add'On.
Projects
None yet
Development

No branches or pull requests

2 participants