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

[Help Wanted/Bug?] Handling of Extra Threads on Plugin Reload #29

Closed
GreenComfyTea opened this issue Feb 26, 2024 · 3 comments
Closed
Labels
managed This issue is related to managed code

Comments

@GreenComfyTea
Copy link

GreenComfyTea commented Feb 26, 2024

Describe The Bug

I don't know if SPL is supposed to do anything with additional threads, but the way it works right now is that if a plugin spawns a new thread, the thread will persist after reloading. New plugin instance will spawn it's own thread too, so as a result there will be 2 threads existing at the same time.

This happens with Task.Run():

Task.Run(() => {
	while (true)
	{
		Log.Info($"Task.Run() Thread: {Thread.CurrentThread.ManagedThreadId}");
	}
});

After 2 reloads:

image

New threads can spawn implicitly. FileSystemWatcher fires it's events in a separate thread, and it persists after a reload too.

The result is this mess:

Screenshot

image

PS. Also it seems that SPL fires events for reloaded plugins before OnLoad(). This does not happen without reloading, when the game is launched from zero.

public void OnLoad()
{
	localizationManager.Init();
	configManager.Init();
}

public void OnImGuiFreeRender()
	{
		// Access data in localizationManager or configManager
		// ---> Exceptions
	}

To Reproduce

  1. Make a plugin with FileSystemWatcher or with Thread.Run() containing repeating console output;
  2. Launch the game;
  3. Hot-reload the plugin;
  4. Observe.

Expected Behavior

If it is a bug with SPL, it should be fixed. Otherwise, maybe there should be IPlugin.OnUnload() event where plugins can dispose of resources and threads, and finalize gracefully.

Priority

Suggested P3 Priority. The issue affects the development of plugins.

PC Specs

Show/Hide
  • Motherboard: Asus ROG Strix Z390-H Gaming
  • CPU: Intel Core i9-9900k
  • RAM: 32 GB 3200MHz
  • GPU: Nvidia GeForce RTX 3090 Ti
  • System and Game SSD: Samsung SSD 970 EVO Plus NVMe M.2 1TB
  • Plugin Repo SSD: Western Digital Blue SATA M.2 2280 2TB

Environment

Show/Hide
  • OS: Window 10 Enterprise Version 22H2 (OS Build 19045.3996)
  • Monster Hunter: World: v15.21.00
  • SharpPluginLoader: pre-v0.0.4.0 (a66b29a)
  • Nvidia Drivers: v551.52

Game Display Settings

Show/Hide
  • DirectX 12 API: On
  • Screen Mode: Borderless Window
  • Resolution Settings: 2880x1620 (Supersampled down to 1920x1080)
  • Aspect Ratio: Wide (16:9)
  • Nvidia DLSS: Off
  • FidelityFX CAS + Upscaling: Off
  • Frame Rate: No Limit
  • V-Sync: On

Game Advanced Graphics Settings

Show/Hide
  • Image Quality: High
  • Texture Quality: High Resolution Texture Pack
  • Ambient Occlusion: High
  • Volume Rendering Quality: Highest
  • Shadow Quality: High
  • Capsule AO: On
  • Contact Shadows: On
  • Anti-Aliasing: Off
  • LOD Bias: High
  • Max LOD Level: No Limit
  • Foliage Sway: On
  • Subsurface Scattering: On
  • Screen Space Reflection: On
  • Anisotropic Filtering: Highest
  • Water Reflection: On
  • Snow Quality: High
  • SH Diffuse Quality: High
  • Dynamic Range: 64-bit
  • Motion Blur: On
  • DOF (Depth of Field): On
  • Vignette Effects: Normal
  • Z-Prepass: On

Mods and External Tools:

Show/Hide

Additional Context

Plugin Repo

@Fexty12573
Copy link
Owner

Good point. I never even thought about that. There is an existing system that goes over all fields in a plugin, and for any that implement IDisposable it calls Dispose on it. So for Task.Run, storing the resulting task as a field in your plugin should result in it being cleaned up. However this does not work for System.Thread.

So yes, perhaps having an OnUnload event like you suggested would be beneficial.

Regarding events being fired pre-OnLoad. Thanks for reporting that. I will change that so no events are called before OnLoad completes. (With the exception of OnPreMain and OnWinMain)

@Fexty12573
Copy link
Owner

Actually, it seems calling Dispose on a Task does not terminate it. So that OnUnload event will be needed either way.

@Fexty12573 Fexty12573 added the managed This issue is related to managed code label Feb 29, 2024
@Fexty12573
Copy link
Owner

OnUnload event was implemented in aff54da. Will close once 0.0.4 is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
managed This issue is related to managed code
Projects
None yet
Development

No branches or pull requests

2 participants