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

Memory leak in GLWpfControl (when creating multiple windows with GL WPF Controls) #126

Closed
4nonym0us opened this issue May 29, 2024 · 4 comments · Fixed by #86
Closed

Memory leak in GLWpfControl (when creating multiple windows with GL WPF Controls) #126

4nonym0us opened this issue May 29, 2024 · 4 comments · Fixed by #86
Labels
4.0 bug Something isn't working
Milestone

Comments

@4nonym0us
Copy link

I've been besting GLWpfControl in a modal WPF Windows, which are opened/closed by user on demand. I've noticed that despite DxGlContext implementing IDisposable, it's never actually called and leads to a memory leak, which becomes more severe if GLWpfControl has short lifespan (i.e. when displayed in a modal dialog/popup).

Steps to reproduce

Modifying the Example project from the repo to instantiate, open and close multiple windows (that contain GLWpfControl) in a loop leads to permanent growth of unmanaged memory on heap, which is never reclaimed/freed.

App.xaml (remove StartupUri)

<Application
    x:Class="Example.App"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml">
    <Application.Resources />
</Application>

App.xaml.cs (change ShutdownMode, create a few windows, force GC)

using System;
using System.Threading.Tasks;
using System.Windows;

namespace Example {
    /// <summary>
    ///     Interaction logic for App.xaml
    /// </summary>
    public partial class App : Application {
        protected override async void OnStartup(StartupEventArgs e) {
            base.OnStartup(e);

            ShutdownMode = ShutdownMode.OnExplicitShutdown;

            Test();

            while (true) {
                await Task.Delay(1000);

                GC.Collect();
                GC.WaitForPendingFinalizers();
            }
        }

        private static void Test() {
            for (var i = 0; i < 10; i++) {
                var window = new MainWindow();
                window.Show();
                window.Close();
            }
        }
    }
}

Following screenshot displays that once the memory is allocated on Window creation, it's never released.
image

Issues

From what It looks like, there are multiple issues:

  1. Having RegisterToEventsDirectly set to True in GLWpfControl leads to the invocation of EventManager.RegisterClassHandler(..) in GLWpfControl.Start(GLWpfControlSettings settings), which creates a strong reference between EventHandler and the control, which make GC keep such instance of GLWpfControl on heap forever even if the control is effectively dead.
  2. DxGlContext is never actually disposed by a developer, but GC won't properly handle it's disposal either because DxGlContext is missing a finalizer.
  3. Resources associated with DxDeviceHandle/DxContextHandle/GlDeviceHandle aren't handled in Dispose() method, which means that even if finalizer was present, resources associated with these handles wouldn't be released anyways and all of these resource seem to be control-specific and not shared (like GraphicsContext/_sharedContextResources).
  4. Current implementation of DxGlContext.Dispose() makes an attempt to dispose resources located in _sharedContextResources, but hence DxGlContext is never properly disposed and finalizer would be required to release resources in this case, the finalizer wouldn't have access to resources in _sharedContextResources because it would likely use non-UI thread, which would result in System.InvalidOperationException: 'The calling thread cannot access this object because a different thread owns it.'.

Discussion

Proper solution of these issues requires implementing IDisposable pattern correctly and disposing used resources.
I'm not aware of all use cases of GLWpfControl and not sure what would be the best approach for this component, but in the worst case scenario, GLWpfControl should have at least a proper finalizer.
I'm open to submit a PR with a fix, but I'd like to know what's the vision of maintainers first (whether to just add finalizer or revise the component to properly utilize IDisposable? How to handle RegisterToEventsDirectly?).

Following screenshot displays the outcome of adding a finalizer to the GLWpfControl and setting RegisterToEventsDirectly: False, which allows GC to take care of the leaked resources.
image
Source code is available here, you can run Example project to reproduce the test results

@NogginBops
Copy link
Member

Great timing actually, I was just working on fixing some stuff related to these kinds of things in #86.
I recently decided to make a push to finish that PR as it's long overdue and would be really nice to get merged.
Will probably merge it sometime tomorrow, and I'll maybe even be able to do a new release of it too.

@NogginBops NogginBops added bug Something isn't working 4.0 labels May 30, 2024
@NogginBops NogginBops added this to the 4.3.0 milestone May 30, 2024
@4nonym0us
Copy link
Author

@NogginBops thanks for looking into it. I definitely like what I see at master...NogginBops:GLWpfControl:cleanup: implementing IDisposable for the Renderer and the actual Control is what makes the most sense and I would consider this the best approach too. In addition, revised design of DxGlContext also looks much cleaner.

My only concern is that when RegisterToEventsDirectly: True is used (which is default setting), it would likely prevent the control from being disposed by GC if developer doesn't correctly handle disposal of GLWpfControl on their end, which is likely to happen because currently GLWpfControl doesn't implement Disposable and many people would just blindly update the NuGet package to a new version without completely realizing the implications.

In the context of issue stated above, it might make sense to:

  1. Add a finalizer to GLWpfControl as a safety net.
  2. Figure out a way to unregister class handler (perhaps when Unloaded even is called) so GC can identify dead controls and utilize that safety net.

@NogginBops
Copy link
Member

I've decided to remove RegisterToEventsDirectly as I was able to find a better solution by setting Focusable=true by default. Will change the readme to reflect this and describe how it registering can be done by the user instead. So there should be no memory leaks left after #86 is merged.

@4nonym0us
Copy link
Author

@NogginBops Sounds great, thanks for working this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants