[GH-ISSUE #5] Avoid locking plugin DLLs when loading #1

New Issue

@Stone-Red-Code commented on GitHub (Oct 25, 2024):

Hi there,

I'm actually aware of this, it's also a problem when you are trying to uninstall a loaded plugin because DM can't delete the files.

This is a problem since the last updated that fixed another issue causing plugin dependencies to not being loaded.
Before that update, the DLL files got loaded into memory, so the file wouldn't be locked. But since that prevented dependencies from loading correctly, I had to use another method that locks the files.

I also looked into shadow copying, but wasn't able to find a reasonable way to implement it.

As you mentioned, a third-party library is likely the best option. This probably would also simplify the whole plugin system, but would obviously require a rewrite of that system.

@Stone-Red-Code commented on GitHub (Oct 25, 2024): Hi there, I'm actually aware of this, it's also a problem when you are trying to uninstall a loaded plugin because DM can't delete the files. This is a problem since the last updated that fixed another issue causing plugin dependencies to not being loaded. Before that update, the DLL files got loaded into memory, so the file wouldn't be locked. But since that prevented dependencies from loading correctly, I had to use another method that locks the files. I also looked into shadow copying, but wasn't able to find a reasonable way to implement it. As you mentioned, a third-party library is likely the best option. This probably would also simplify the whole plugin system, but would obviously require a rewrite of that system.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Oct 25, 2024):

I will try to find a library that would work with the DM plugin system and try to implement it as soon as possible.

DotNetCorePlugins looks promising.

@Stone-Red-Code commented on GitHub (Oct 25, 2024): I will try to find a library that would work with the DM plugin system and try to implement it as soon as possible. [DotNetCorePlugins](https://github.com/natemcmaster/DotNetCorePlugins) looks promising.

Stone_Red commented

@dylanrenwick commented on GitHub (Oct 25, 2024):

Loading the file into memory to avoid locking the file-on-disk was my first thought, so it's good to know that's been tried and ruled out. I hadn't considered plugin dependencies.
I'll do some research into and testing of shadow copy implementations and/or third-party libraries and see if I can find a good solution for DesktopMagic. DotNetCorePlugins does look very promising, hopefully it lives up to that promise!

I'm excited about the prospect of a C#/dotnet oriented replacement for Rainmeter (who thought object-oriented ini files were a good idea?) so I'm looking to contribute a bit 😄

@dylanrenwick commented on GitHub (Oct 25, 2024): Loading the file into memory to avoid locking the file-on-disk was my first thought, so it's good to know that's been tried and ruled out. I hadn't considered plugin dependencies. I'll do some research into and testing of shadow copy implementations and/or third-party libraries and see if I can find a good solution for DesktopMagic. DotNetCorePlugins does look very promising, hopefully it lives up to that promise! I'm excited about the prospect of a C#/dotnet oriented replacement for Rainmeter (who thought object-oriented ini files were a good idea?) so I'm looking to contribute a bit 😄

Stone_Red commented

@Stone-Red-Code commented on GitHub (Oct 29, 2024):

It seems like the maintainer of DotNetCorePlugins doesn't want to maintain the library anymore and is considering archiving the repository. So that doesn't seem like a viable option.

The biggest problem with loading assemblies in .NET is unloading them.
The AssemblyLoadContext class should theoretically solve this problem, but I haven't played around with it yet.

Another option would be to load the plugins into a separate process that communicates with IPC with the main process.
This would make it very easy to unload the assembly because we can just stop/kill the process if required. And it would also solve some performance issues that are caused by WPF because it requires all UI interactions to run on the main UI thread. (The main window and other plugins can stutter when plugins are updating the Bitmap)

Both of these ideas would require some form of shadow copying because of the Assembly.Load method would still lock the files.

@Stone-Red-Code commented on GitHub (Oct 29, 2024): It seems like the maintainer of DotNetCorePlugins doesn't want to maintain the library anymore and is considering archiving the repository. So that doesn't seem like a viable option. The biggest problem with loading assemblies in .NET is unloading them. The `AssemblyLoadContext` class should theoretically solve this problem, but I haven't played around with it yet. Another option would be to load the plugins into a separate process that communicates with IPC with the main process. This would make it very easy to unload the assembly because we can just stop/kill the process if required. And it would also solve some performance issues that are caused by WPF because it requires all UI interactions to run on the main UI thread. (The main window and other plugins can stutter when plugins are updating the `Bitmap`) Both of these ideas would require some form of shadow copying because of the `Assembly.Load` method would still lock the files.

Stone_Red commented

@dylanrenwick commented on GitHub (Nov 1, 2024):

It's looking to me like we may need to implement shadow copying ourselves. I'm not finding many good options that are still maintained.

I'll take a stab over the weekend at implementing a standalone lib that can load an Assembly from a file without locking it, whilst still maintaining proper Load Context and dependencies

@dylanrenwick commented on GitHub (Nov 1, 2024): It's looking to me like we may need to implement shadow copying ourselves. I'm not finding many good options that are still maintained. I'll take a stab over the weekend at implementing a standalone lib that can load an `Assembly` from a file without locking it, whilst still maintaining proper Load Context and dependencies

Stone_Red commented

@dylanrenwick commented on GitHub (Nov 1, 2024):

After some brief testing, it seems the solution may actually be as simple as copying the target file before loading it.
I created a simple proof-of-concept here to demonstrate this.
All it does is copy the .dll we want to load to a local _shadow directory before loading it. To my surprise this doesn't seem to break dependency resolution at all, and even appears to avoid locking loaded dependencies (Dependency.dll was not locked, even before the test project itself depended on it)

If you can think of a more complex real world use case/edge case that this PoC doesn't cover let me know and I'll add it to the test suite.

@dylanrenwick commented on GitHub (Nov 1, 2024): After some brief testing, it seems the solution may actually be as simple as copying the target file before loading it. I created a simple proof-of-concept [here](https://github.com/dylanrenwick/PluginLoader) to demonstrate this. All it does is copy the `.dll` we want to load to a local `_shadow` directory before loading it. To my surprise this doesn't seem to break dependency resolution at all, and even appears to avoid locking loaded dependencies (`Dependency.dll` was not locked, even before the test project itself depended on it) If you can think of a more complex real world use case/edge case that this PoC doesn't cover let me know and I'll add it to the test suite.

Stone_Red commented

@dylanrenwick commented on GitHub (Nov 1, 2024):

Additionally, it looks like if we load each plugin into its own AssemblyLoadContext and ensure they're set to collectible when created, we can unload plugins at runtime with AssemblyLoadContext.Unload().
Only whole LoadContexts can be unloaded though, not individual plugins, so each plugin would need its own LoadContext. I don't think this would cause problems, but it may warrant further investigation.

@dylanrenwick commented on GitHub (Nov 1, 2024): Additionally, it looks like if we load each plugin into its own `AssemblyLoadContext` and ensure they're set to collectible when created, we can unload plugins at runtime with `AssemblyLoadContext.Unload()`. Only whole LoadContexts can be unloaded though, not individual plugins, so each plugin would need its own LoadContext. I don't _think_ this would cause problems, but it may warrant further investigation.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Nov 1, 2024):

Your PoC looks good, but are the dependencies not copied into the _shadow directory?
When removing the Dependency project from the project references of the PluginDotNet.Test project, the DependentPlugin.dll fails to load because it can't find Dependency.dll

Additionally, it looks like if we load each plugin into its own AssemblyLoadContext and ensure they're set to collectible when created, we can unload plugins at runtime with AssemblyLoadContext.Unload(). Only whole LoadContexts can be unloaded though, not individual plugins, so each plugin would need its own LoadContext. I don't think this would cause problems, but it may warrant further investigation.

Yeah, that's how the AssemblyLoadContext is supposed to be used. Loading each plugin into its own AssemblyLoadContext shouldn't be a problem.

@Stone-Red-Code commented on GitHub (Nov 1, 2024): Your PoC looks good, but are the dependencies not copied into the `_shadow` directory? When removing the `Dependency` project from the project references of the `PluginDotNet.Test` project, the `DependentPlugin.dll` fails to load because it can't find `Dependency.dll` > Additionally, it looks like if we load each plugin into its own `AssemblyLoadContext` and ensure they're set to collectible when created, we can unload plugins at runtime with `AssemblyLoadContext.Unload()`. Only whole LoadContexts can be unloaded though, not individual plugins, so each plugin would need its own LoadContext. I don't _think_ this would cause problems, but it may warrant further investigation. Yeah, that's how the `AssemblyLoadContext` is supposed to be used. Loading each plugin into its own `AssemblyLoadContext ` shouldn't be a problem.

Stone_Red commented

@dylanrenwick commented on GitHub (Nov 1, 2024):

That's odd. I had run the DependencyPlugin test before adding the project reference and it had worked fine. I added the project reference solater that I could add the interface check.

I admittedly didn't check if the dependencies were also copied into _shadow, though I don't see that being an issue as we would probably need the dependencies shadow copied as well anyway, or at the very least not locked.

For that reason it may also make sense to give each loaded plugin its own subdirectory, to avoid file conflicts.

My one concern with this approach 8s shared dependencies, ie if multiple plugins reference the same dependency (is each plugin going to need its own PluginAPI assembly?)
I'll do some more testing and double check my results.

@dylanrenwick commented on GitHub (Nov 1, 2024): That's odd. I had run the `DependencyPlugin` test before adding the project reference and it had worked fine. I added the project reference solater that I could add the interface check. I admittedly didn't check if the dependencies were also copied into `_shadow`, though I don't see that being an issue as we would probably need the dependencies shadow copied as well anyway, or at the very least not locked. For that reason it may also make sense to give each loaded plugin its own subdirectory, to avoid file conflicts. My one concern with this approach 8s shared dependencies, ie if multiple plugins reference the same dependency (is each plugin going to need its own PluginAPI assembly?) I'll do some more testing and double check my results.

Stone_Red commented

@dylanrenwick commented on GitHub (Nov 8, 2024):

Running some further tests I was able to confirm your results; that the DependencyPlugin test fails if the test suite doesn't itself has a reference to the dependency.

Unfortunately I can't see a great way around this as there doesn't seem to be a good way to detect an assembly's dependencies before loading in a way that allows us to shadow copy and load them as well.

I'm going to keep investigating this, and look into how DotNetCorePlugins handled dependencies, but in the meantime an interrim solution may be to update the current plugin loading to use the AssemblyLoadContext approach, and provide an option on the UI to unload a plugin without having to restart DM itself.

This would still be an improvement over the current development cycle as you would just need to unload a single plugin, update the files, then reload

@dylanrenwick commented on GitHub (Nov 8, 2024): Running some further tests I was able to confirm your results; that the `DependencyPlugin` test fails if the test suite doesn't itself has a reference to the dependency. Unfortunately I can't see a great way around this as there doesn't seem to be a good way to detect an assembly's dependencies before loading in a way that allows us to shadow copy and load them as well. I'm going to keep investigating this, and look into how DotNetCorePlugins handled dependencies, but in the meantime an interrim solution may be to update the current plugin loading to use the `AssemblyLoadContext` approach, and provide an option on the UI to unload a plugin without having to restart DM itself. This would still be an improvement over the current development cycle as you would just need to unload a single plugin, update the files, then reload

Stone_Red commented

@Stone-Red-Code commented on GitHub (Nov 12, 2024):

Thanks for looking into this.
Can't we just load the dependencies into the AssemblyLoadContext? If I understood that correctly AssemblyLoadContexts don't share dependencies by default.

@Stone-Red-Code commented on GitHub (Nov 12, 2024): Thanks for looking into this. Can't we just load the dependencies into the `AssemblyLoadContext`? If I understood that correctly `AssemblyLoadContext`s don't share dependencies by default.

Stone_Red commented

@dylanrenwick commented on GitHub (Nov 14, 2024):

We can. My concern is that we don't know what dependencies a plugin has without loading it, so when shadow copying the plugin assembly we don't know what dependencies also need to be shadow copied.

If we shadow copy the plugin assembly and not its dependencies, the library will fail to load with unresolved dependencies.

@dylanrenwick commented on GitHub (Nov 14, 2024): We can. My concern is that we don't know what dependencies a plugin has without loading it, so when shadow copying the plugin assembly we don't know what dependencies also need to be shadow copied. If we shadow copy the plugin assembly and not its dependencies, the library will fail to load with unresolved dependencies.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Nov 14, 2024):

Well the it's already a requirement that all plugins have to include all the necessary dependencies in the zip file/plugin directory.

So we can just copy the whole directory and load everything necessary.

@Stone-Red-Code commented on GitHub (Nov 14, 2024): Well the it's already a requirement that all plugins have to include all the necessary dependencies in the zip file/plugin directory. So we can just copy the whole directory and load everything necessary.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Dec 9, 2024):

Hi, any updates?
If not, I will implement a temporary solution to at least make plugin development less frustrating.

@Stone-Red-Code commented on GitHub (Dec 9, 2024): Hi, any updates? If not, I will implement a temporary solution to at least make plugin development less frustrating.

Stone_Red commented

@dylanrenwick commented on GitHub (Dec 12, 2024):

Hi, sorry I haven't had much time to work on it the last couple of weeks with holidays coming up.

I have a local fork that isolates each plugin assembly into its own AssemblyLoadContext, and loads the assemblies from memory instead of from file.

This prevents the files being locked, and allows DM to reload the plugin assembly from file each time it's enabled, making development smoother.

However, when unloading the load contexts on plugin disable, the load context isn't being cleaned up properly and I haven't been successful in figuring out why. This means each time you toggle a plugin the fork leaks around 80kb of memory in the form of an orphaned load context.

@dylanrenwick commented on GitHub (Dec 12, 2024): Hi, sorry I haven't had much time to work on it the last couple of weeks with holidays coming up. I have a local fork that isolates each plugin assembly into its own `AssemblyLoadContext`, and loads the assemblies from memory instead of from file. This prevents the files being locked, and allows DM to reload the plugin assembly from file each time it's enabled, making development smoother. However, when unloading the load contexts on plugin disable, the load context isn't being cleaned up properly and I haven't been successful in figuring out why. This means each time you toggle a plugin the fork leaks around 80kb of memory in the form of an orphaned load context.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Dec 12, 2024):

No worries

After looking through the plugin code, I noticed multiple possible references that could prevent the assembly from unloading.
There's the PluginData class that gets passed to the plugin, but the bigger problem is probably the references to the Settings objects.
DM has a reference to each Setting object that's defined in the Plugin class.

BTW is your fork on GH up to date?

@Stone-Red-Code commented on GitHub (Dec 12, 2024): No worries After looking through the plugin code, I noticed multiple possible references that could prevent the assembly from unloading. There's the `PluginData` class that gets passed to the plugin, but the bigger problem is probably the references to the `Setting`s objects. DM has a reference to each `Setting` object that's defined in the `Plugin` class. BTW is your fork on GH up to date?

Stone_Red commented

@dylanrenwick commented on GitHub (Dec 19, 2024):

It wasn't up to date, but I just pushed my changes to it.
I did catch the Setting references, and tried to encapsulate that in my load/unload logic.
dotSpy doesn't find any references to the AssemblyLoadContext instances, but they don't get cleaned up by GC regardless.

@dylanrenwick commented on GitHub (Dec 19, 2024): It wasn't up to date, but I just pushed my changes to it. I did catch the `Setting` references, and tried to encapsulate that in my load/unload logic. dotSpy doesn't find any references to the `AssemblyLoadContext` instances, but they don't get cleaned up by GC regardless.

Stone_Red commented

@dylanrenwick commented on GitHub (Feb 25, 2025):

Finally found the time to revisit this.
Looking back through my fork changes, I had separated the actual plugin loading logic out into its own class, because IMO it's not ideal to have that logic directly in the WPF code-behind (for best practice reasons).

The isolated PluginLoader class also uses AssemblyLoadContext over Assembly.Load as discussed above, isolating each plugin into its own context.
I've been running this fork for a while at work now, though I've only been running the built-in date/time and CPU usage plugins, alongside my own CPU/RAM usage bars plugin.

If you're on-board with the separation of concerns with plugin loading, I'll do some more thorough testing with other publicly available plugins to make sure everything's working as expected, and submit a PR. If you're unsure but want to look through the changes I can still submit a draft PR for your review.

Notably I have yet to track down the cause of the load context memory leak (AssemblyLoadContext instances are not being GC'd despite no detectable references to them), but given that's only a concern when unloading a plugin and is still appropriately cleaned up when DM is closed, my suggestion would be that we merge the changes to resolve this issue, and create a new issue to track investigation of the memory leak. As far as I can tell it looks like the leak is on the order of a few hundred bytes at most each time a plugin is unloaded which, while not ideal, isn't a major concern IMO.

Let me know how you want to proceed.

@dylanrenwick commented on GitHub (Feb 25, 2025): Finally found the time to revisit this. Looking back through my fork changes, I had separated the actual plugin loading logic out into its own class, because IMO it's not ideal to have that logic directly in the WPF code-behind (for best practice reasons). The isolated PluginLoader class also uses `AssemblyLoadContext` over `Assembly.Load` as discussed above, isolating each plugin into its own context. I've been running this fork for a while at work now, though I've only been running the built-in date/time and CPU usage plugins, alongside my own CPU/RAM usage bars plugin. If you're on-board with the separation of concerns with plugin loading, I'll do some more thorough testing with other publicly available plugins to make sure everything's working as expected, and submit a PR. If you're unsure but want to look through the changes I can still submit a draft PR for your review. Notably I have yet to track down the cause of the load context memory leak (`AssemblyLoadContext` instances are not being GC'd despite no detectable references to them), but given that's only a concern when unloading a plugin and is still appropriately cleaned up when DM is closed, my suggestion would be that we merge the changes to resolve this issue, and create a new issue to track investigation of the memory leak. As far as I can tell it looks like the leak is on the order of a few hundred bytes at most each time a plugin is unloaded which, while not ideal, isn't a major concern IMO. Let me know how you want to proceed.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Feb 25, 2025):

Looking back through my fork changes, I had separated the actual plugin loading logic out into its own class, because IMO it's not ideal to have that logic directly in the WPF code-behind (for best practice reasons).

Yeah, that does indeed make sense, I mean some code in this project is ~5 years old. Some refactoring won't hurt. xD

If you're on-board with the separation of concerns with plugin loading, I'll do some more thorough testing with other publicly available plugins to make sure everything's working as expected, and submit a PR. If you're unsure but want to look through the changes I can still submit a draft PR for your review.

If you can get the Media Display plugin to load and work as intended, then your implementation should be fine.
And yeah, just submit a draft PR.

As far as I can tell it looks like the leak is on the order of a few hundred bytes at most each time a plugin is unloaded which, while not ideal, isn't a major concern IMO.

That's also what I would have suggested. The memory leak probably isn't a big deal.

So, do some more testing and submit a PR whenever you’re good to go.

@Stone-Red-Code commented on GitHub (Feb 25, 2025): > Looking back through my fork changes, I had separated the actual plugin loading logic out into its own class, because IMO it's not ideal to have that logic directly in the WPF code-behind (for best practice reasons). Yeah, that does indeed make sense, I mean some code in this project is ~5 years old. Some refactoring won't hurt. xD > If you're on-board with the separation of concerns with plugin loading, I'll do some more thorough testing with other publicly available plugins to make sure everything's working as expected, and submit a PR. If you're unsure but want to look through the changes I can still submit a draft PR for your review. If you can get the [Media Display](https://mod.io/g/desktopmagic/m/media-display1) plugin to load and work as intended, then your implementation should be fine. And yeah, just submit a draft PR. > As far as I can tell it looks like the leak is on the order of a few hundred bytes at most each time a plugin is unloaded which, while not ideal, isn't a major concern IMO. That's also what I would have suggested. The memory leak probably isn't a big deal. So, do some more testing and submit a PR whenever you’re good to go.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Mar 25, 2025):

Hi there, I checked out your plugin loader code.

Dependencies weren't loaded correctly but after adding

loadContext.Resolving += LoadContext_Resolving;

and

Assembly? LoadContext_Resolving(AssemblyLoadContext context, AssemblyName assemblyName)
{
    string assemblyPath = Path.Combine(pluginData.DirectoryPath, assemblyName.Name + ".dll");
    if (!File.Exists(assemblyPath))
    {
        return null;
    }
    return context.LoadFromAssemblyPath(assemblyPath);
}

to PluginLoader.cs dependencies seemed to load fine.
I also haven't tested everything yet, though.

@Stone-Red-Code commented on GitHub (Mar 25, 2025): Hi there, I checked out your plugin loader code. Dependencies weren't loaded correctly but after adding ```cs loadContext.Resolving += LoadContext_Resolving; ``` and ```cs Assembly? LoadContext_Resolving(AssemblyLoadContext context, AssemblyName assemblyName) { string assemblyPath = Path.Combine(pluginData.DirectoryPath, assemblyName.Name + ".dll"); if (!File.Exists(assemblyPath)) { return null; } return context.LoadFromAssemblyPath(assemblyPath); } ``` to `PluginLoader.cs` dependencies seemed to load fine. I also haven't tested everything yet, though.

Stone_Red commented

@dylanrenwick commented on GitHub (Mar 25, 2025):

Ah nice catch!

I've been trying to find the free time to test this at home for a few weeks now, early march has been kinda crazy.
I have carved out some time tonight to give it a proper test though.
Unfortunately I mainly use DM at work, in a VM with no audio output or graphics device, so I've been trying to find the opportunity to test it at home on my desktop.

I'll return tonight or tomorrow morning with the test results. I imagine it should all work especially with your dependency fixes, but best to make sure.

@dylanrenwick commented on GitHub (Mar 25, 2025): Ah nice catch! I've been trying to find the free time to test this at home for a few weeks now, early march has been kinda crazy. I have carved out some time tonight to give it a proper test though. Unfortunately I mainly use DM at work, in a VM with no audio output or graphics device, so I've been trying to find the opportunity to test it at home on my desktop. I'll return tonight or tomorrow morning with the test results. I imagine it should all work especially with your dependency fixes, but best to make sure.

Stone_Red commented

@dylanrenwick commented on GitHub (Mar 26, 2025):

Ran some tests with the media display plugin and everything seemed to work fine. I was able to play audio and video files with no issues.

I'll commit your dependency fixes into my fork and submit a PR today

@dylanrenwick commented on GitHub (Mar 26, 2025): Ran some tests with the media display plugin and everything seemed to work fine. I was able to play audio and video files with no issues. I'll commit your dependency fixes into my fork and submit a PR today

Stone_Red commented

@Stone-Red-Code commented on GitHub (Apr 6, 2025):

How's it going?

@Stone-Red-Code commented on GitHub (Apr 6, 2025): How's it going?

Stone_Red commented

@dylanrenwick commented on GitHub (Apr 11, 2025):

I ran some more tests before opening the PR, and Media Display Plugin wasn't working. It worked fine on the main branch of DM, but not my fork. It displays fine, but won't detect any media playing. I haven't had the chance to look into it too much yet but I'm going to have a good deal more free time after next week so I'm planning to investigate then.

@dylanrenwick commented on GitHub (Apr 11, 2025): I ran some more tests before opening the PR, and Media Display Plugin wasn't working. It worked fine on the main branch of DM, but not my fork. It displays fine, but won't detect any media playing. I haven't had the chance to look into it too much yet but I'm going to have a good deal more free time after next week so I'm planning to investigate then.

Stone_Red commented

@Stone-Red-Code commented on GitHub (Dec 19, 2025):

My implementation in https://github.com/Stone-Red-Code/DesktopMagic/commit/780b5061039c40d0db9d7e32255c81dc462d6da5 seems to work and is surprisingly simple.

@Stone-Red-Code commented on GitHub (Dec 19, 2025): My implementation in https://github.com/Stone-Red-Code/DesktopMagic/commit/780b5061039c40d0db9d7e32255c81dc462d6da5 seems to work and is surprisingly simple.

Stone_Red commented