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

Any way to revert to pre-7.0 isolated assembly loading in C++/CLI? #104480

Closed
mikeoliphant opened this issue Jul 5, 2024 · 28 comments · Fixed by #105337
Closed

Any way to revert to pre-7.0 isolated assembly loading in C++/CLI? #104480

mikeoliphant opened this issue Jul 5, 2024 · 28 comments · Fixed by #105337
Milestone

Comments

@mikeoliphant
Copy link
Contributor

I have a framework using C++/CLI to load managed plugins within a native application.

This change to loading assemblies in the default ALC:

#66486

has broken my ability to load multiple plugins.

Is there any way to revert to the previous isolated assembly loading behavior?

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 5, 2024
Copy link
Contributor

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

@mikeoliphant
Copy link
Contributor Author

To add a bit more information, what I have is:

  • A native application host that loads native dll plugins (a VST3 audio plugin host, to be specific).
  • Managed plugins wrapped in a C++/CLI wrapper framework to act as the native interface with the native host.

So, a "plugin" consists of a folder with the C++/CLI wrapper dll which loads the actual managed plugin code.

I can load a single plugin just fine. I can also load the same plugin multiple times from the same folder. Trying to load a second, different plugin, or even the same plugin, but from a different folder, no longer works (it does in .NET 6.0).

@AaronRobinsonMSFT
Copy link
Member

@mikeoliphant There is no current way to revert to the previous C++/CLI behavior.

It would be helpful to get a trace - see instructions here.

/cc @elinor-fung

@mikeoliphant
Copy link
Contributor Author

Here is a trace:

vsthost.exe_20240705_181811.zip

The application activity was:

  • load a plugin
  • load the same plugin again from the same location
  • load the same plugin again, but from a different path

When the plugin is loaded from a different path, everything goes haywire and the trace keeps getting bigger and bigger. The one I linked above I cut off quickly to keep it from getting too big.

From looking at it with perfview (my first time with the tool, so I don't really know what I'm doing) it looks like it is loading the assembly "AudioPlugSharpVst" over and over again. That assembly is my C++/CLI assembly which is the entry point from the native code.

@AaronRobinsonMSFT
Copy link
Member

@mikeoliphant Yep, I see the behavior you are referring to. The solution here is to have a different C++/CLI assembly. We changed this behavior as it more aligned with the majority of C++/CLI use cases coming from .NET Framework. Is it possible for you to adapt to the new behavior?

@mikeoliphant
Copy link
Contributor Author

This is my github repo:

https://github.com/mikeoliphant/AudioPlugSharp

The C++/CLI assembly is a framework for loading managed plugins from native code (specifically, VST3 audio plugins).

If I'm understanding you correctly, I don't see how it would work for me to have different C++/CLI assemblies per plugin.

@AaronRobinsonMSFT
Copy link
Member

Oh, I see. You provide a single C++/CLI assembly and anyone can drop that next to their plug-in written in .NET and it will get loaded, right?

@mikeoliphant
Copy link
Contributor Author

Oh, I see. You provide a single C++/CLI assembly and anyone can drop that next to their plug-in written in .NET and it will get loaded, right?

Yep, that's exactly it.

@AaronRobinsonMSFT
Copy link
Member

I see. This is going to require feedback from @elinor-fung. My gut tells me this isn't something we are likely to add support for, but I'll defer to the hosting experts.

@mikeoliphant
Copy link
Contributor Author

My gut tells me this isn't something we are likely to add support for, but I'll defer to the hosting experts.

If so, that would be very disappointing if I have no possible way to work around it.

Can you give me some insight as to why it isn't working? My code should work in both the case that the separate instances of the C++/CLI assembly are loaded independently and isolated, and also in the case that only one copy is loaded and used.

@agocke agocke added this to the Future milestone Jul 8, 2024
@agocke agocke removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2024
@elinor-fung
Copy link
Member

I actually wouldn't be opposed to adding a switch to allow using an isolated context. We had considered it when switching to the default, but didn't as C++/CLI support was very much targeted at .NET Framework migration.

As for why it isn't working - how does the failure to load manifest? When trying to load the plugin from a different path, the runtime should find that AudioPlugSharpVst is already loaded in the default load context and use that assembly (that is also what your trace seems to confirm). From a quick look at the code, it seems like it is relying on its location and file name to find the actual plugin assembly to load, so it wouldn't try to load the right one?

@elinor-fung
Copy link
Member

I actually wouldn't be opposed to adding a switch to allow using an isolated context. We had considered it when switching to the default, but didn't as C++/CLI support was very much targeted at .NET Framework migration.

I think the actual work would be similar to the configuration we added in .NET 8 for loading COM components in the default ALC. I don't think it should be too complicated as there's an existing model for it.

If you are interested or have time to take a stab at it, I can point at what I expect needs to happen and we'd welcome contributions.

@mikeoliphant
Copy link
Contributor Author

As for why it isn't working - how does the failure to load manifest?

It has been hard for me to track down, since when the error occurs it isn't writing anything to my log and I'm not sure how to attach a debugger given the execution environment.

From a quick look at the code, it seems like it is relying on its location and file name to find the actual plugin assembly to load, so it wouldn't try to load the right one?

Ah, when it is being loaded a second time from a different location, it keeps the existing assembly and "Assembly.GetExecutingAssembly().Location" will return the original assemblies location? That would definitely cause problems, since that is the only way I know where my other assemblies are located. Although it seems like that might not explain why it has trouble with the same plugin loaded a second time from a different folder - I would think it would just load again from the first folder?

If you are interested or have time to take a stab at it, I can point at what I expect needs to happen and we'd welcome contributions.

My quickest path to success is probably going to be finding a way to make things work with the new existing behavior, but failing that (or in addition to it) I'm certainly willing to put some time into working on adding a switch to re-enable the previous assembly loading behavior.

@mikeoliphant
Copy link
Contributor Author

Looking at this more closely, I don't think that it is even getting to my code when trying to load a plugin a second time from a different location.

Looking at the trace, I think you can see it succeeding the first two times (both loading from the same place) at around 1:18:18 and 1:18:24.

Then I try to load it from a different path (around 1:18:39) and it doesn't get past loading the main C++/CLI assembly ("AudioPlugSharpVst"). My actual plugin loading code is in a referenced assembly - "AudioPlugSharp" - and I don't think it even gets there. It just keeps loading "AudioPluginSharpVst" over and over again.

@elinor-fung
Copy link
Member

Ah, thanks for pointing that out - I was able to look at a repro myself.

On load of a C++/CLI assembly, ijwhost patches the assembly's .vtfixup table to point at stubs in ijwhost. When called, those stubs will start the runtime, load the managed assembly into the runtime, and patch the .vtfixup table again to point to JIT stubs.

So in the scenario of loading the same C++/CLI assembly from two different paths with the same AudioPlugSharpVst managed assembly name (in metadata):

  • Load and call into function from C++/CLI assembly in path A:
    • ijwhost stub is called
    • runtime is started
    • runtime loads managed assembly AudioPlugSharpVst
      • it is associated with the image at path A
    • runtime patches the .vtfixup table tokens to point at JIT stubs corresponding to the managed method from the managed assembly
    • JIT stub is called and the managed method is executed
  • Load and call into function from same C++/CLI assembly in path B:
    • ijwhost stub is called
    • runtime is detected to already be started
    • runtime determines AudioPlugSharpVst is already loaded
      • it is the one from path A, associated with the image at path A
    • runtime determines the associated image has already been patched, so the .vtfixup table for the image in path B continues to point to the ijwhost stubs
    • ijwhost stub is called again

I think a switch to allow the isolated ALC behaviour may be the best option here.

@mikeoliphant
Copy link
Contributor Author

mikeoliphant commented Jul 11, 2024

I think a switch to allow the isolated ALC behaviour may be the best option here.

That should solve my problem, and would potentially be useful to others.

On the other hand, if, somehow, the behavior could be fixed so that the second assembly load works, but is in a the default ALC (ie: still using the first copy), I think I might be able make things work by getting the plugin location information from the native side rather than the managed side.

I also might be able to work around the currently existing behavior by having another C++ dll as my entry point that always loads the AudioPlugSharpVst dll from a fixed location.

@mikeoliphant
Copy link
Contributor Author

Please let me know if there is anything I can do to help move this forward.

@elinor-fung
Copy link
Member

Here's what I expect needs to happen for a switch:

  1. Make ijwhost check a runtime config property to determine the load context to use:
    [](pal::dll_t fxr, hostfxr_handle context){ },

    This should be similar to what comhost does (but defaulting to using the Default ALC if the property is not set):
    [load_context](pal::dll_t fxr, hostfxr_handle context)
    {
    *load_context = ISOLATED_CONTEXT;
    auto get_runtime_property_value = reinterpret_cast<hostfxr_get_runtime_property_value_fn>(pal::get_symbol(fxr, "hostfxr_get_runtime_property_value"));
    const pal::char_t* value;
    if (get_runtime_property_value(context, _X("System.Runtime.InteropServices.COM.LoadComponentInDefaultContext"), &value) == StatusCode::Success
    && pal::strcasecmp(value, _X("true")) == 0)
    {
    *load_context = nullptr; // Default context
    }
    },
  2. Pass the context determined in (1) when calling the delegate:
    loadInMemoryAssembly(moduleHandle, app_path.c_str(), nullptr);
  3. Update InMemoryAssemblyLoader to use the default or isolated context based on the value of loadContext (IntPtr.Zero for default ALC, value of -1 for isolated context):
    ArgumentOutOfRangeException.ThrowIfNotEqual(loadContext, IntPtr.Zero);
    LoadInMemoryAssemblyInContextImpl(moduleHandle, assemblyPath, AssemblyLoadContext.Default);
  4. Add tests to Ijwhost test class. Existing test already validates that it loads the default ALC, so this should be adding tests for when the property is explicitly set to true/false - similar to the Comhost test.

I don't think our team has the capacity to do this right now, so if you're willing to put some work into adding a switch, we'd welcome it.

@mikeoliphant
Copy link
Contributor Author

Thanks very much for the detailed information - it is very helpful.

@mikeoliphant
Copy link
Contributor Author

@elinor-fung Your instructions are clear, and the changes seem fairly straightforward.

I've cloned the runtime repository and built it. My struggle right now is with getting an environment set up to test changes.

I've got a Core_Root folder created, but I don't know how to make a plugin reference it. I don't think I can use corerun - it seems designed to work with managed assemblies?

I can put my modified ijwhost dll in my plugin folder, and that works, but I don't know how to test that alongside code changes to InMemoryAssemblyLoader.

Any help would be much appreciated.

@mikeoliphant
Copy link
Contributor Author

Another issue I'm having is that I can't select .NET 9.0 as the target framework for my C++/CLI project.

I'm running latest preview version of Visual Studio, and have the 9.0.100-preview.6.24328.19 SDK installed.

@mikeoliphant
Copy link
Contributor Author

I still haven't figured out how to test my plugin code against a dev version of the runtime, but I think I've successfully made the necessary changes to the runtime.

You can see what I've done on this branch:

https://github.com/mikeoliphant/runtime/tree/ijwhost_isolated_switch

There are three commits - one for the InMemoryAssemblyLoader change, one for the ijwhost changes and one for the test.

@elinor-fung
Copy link
Member

I've got a Core_Root folder created, but I don't know how to make a plugin reference it. I don't think I can use corerun - it seems designed to work with managed assemblies?

Ah, yeah, corerun basically replaces regular hosting for the purposes of testing, so it doesn't work so well when you're making changes across all of hosting, libraries, and runtime.

I generally use the libs.pretest subset to create a a folder with my local build and set the DOTNET_ROOT environment variable when running an application to use it:
https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/host/using-apphost.md#pointing-at-a-local-net-root

Another issue I'm having is that I can't select .NET 9.0 as the target framework for my C++/CLI project.

Copying over the modified ijwhost as you already did should allow you to use your locally built host. To make it use your local .NET 9 runtime (after creating it and setting DOTNET_ROOT as above), you can set RollForward to Major: https://learn.microsoft.com/dotnet/core/versions/selection#control-roll-forward-behavior

@mikeoliphant
Copy link
Contributor Author

I generally use the libs.pretest subset to create a a folder with my local build and set the DOTNET_ROOT environment variable when running an application to use it:

Thanks - that worked.

I've now verified that my plugin code works with my runtime changes.

As expected, it works when I add "System.Runtime.InteropServices.IJWHost.LoadComponentInIsolatedContext": true". If that config flag is "false" or absent, I get the previous behavior (assembly load loop when loading a plugin for a second time from a different location).

I think it is all good from my end. Let me know if you have any comments on the changes (for example, on the naming of the config option). Once I get a thumbs-up from you, I'll go ahead and submit a PR.

@elinor-fung
Copy link
Member

Glad to hear it is working for you.

I took a look at your branch - looking good. Some comments:

  • Config name: I'd recommend IJWHost -> CppCLI - so System.Runtime.InteropServices.CppCLI.LoadComponentInIsolatedContext. I think that better matches how we use C++/CLI as the feature name publicly (instead of IJW).
  • Tests: we should have the case of having a config with that property explicitly set to false. I'd find it clearer to add a separate LoadLibrary_ContextConfig(bool loadIsolated) test that always explicitly sets the property to true/false. And then leave the existing LoadLibrary test as validating the default behaviour (config property not set)

@mikeoliphant
Copy link
Contributor Author

  • Config name: I'd recommend IJWHost -> CppCLI - so System.Runtime.InteropServices.CppCLI.LoadComponentInIsolatedContext. I think that better matches how we use C++/CLI as the feature name publicly (instead of IJW).
  • Tests: we should have the case of having a config with that property explicitly set to false. I'd find it clearer to add a separate LoadLibrary_ContextConfig(bool loadIsolated) test that always explicitly sets the property to true/false. And then leave the existing LoadLibrary test as validating the default behaviour (config property not set)

Ok - changes made:

main...mikeoliphant:runtime:ijwhost_isolated_switch

@elinor-fung
Copy link
Member

For the test, it should still always add the property, but set it to "true" or "false" based on the value of load_isolated - so
main...mikeoliphant:runtime:ijwhost_isolated_switch#diff-dbc53d0f5dc8eca45c703e151919a8552fd54eeb96ec4f4b876cc8a0cbc8d061R71-R76 would become something like:

RuntimeConfig.FromFile(app.RuntimeConfigJson)
    .WithProperty("System.Runtime.InteropServices.CppCLI.LoadComponentInIsolatedContext", load_isolated.ToString())
    .Save();

Otherwise, seems good. Please put up a PR whenever you are ready.

@mikeoliphant
Copy link
Contributor Author

For the test, it should still always add the property, but set it to "true" or "false" based on the value of load_isolated

Fixed. PR is here: #105337

Thanks for your help!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
5 participants