Memory Leak

[simon-halsey-fadv]

We've just switched to use Dink instead of Running WkHtml in a process, so to see if it behaves any better.

Whilst it seems to report less errors, there does seem to be a memory leak. I'm wondering if this bit of code in BasicConverter shouldn't have a try finally?

        ProcessingDocument = document;
        byte[] result = new byte[0];
        Tools.Load();
        IntPtr converter = CreateConverter(document);
        Tools.SetPhaseChangedCallback(converter, OnPhaseChanged);
        Tools.SetProgressChangedCallback(converter, OnProgressChanged);
        Tools.SetFinishedCallback(converter, OnFinished);
        Tools.SetWarningCallback(converter, OnWarning);
        Tools.SetErrorCallback(converter, OnError);
        if (Tools.DoConversion(converter))
        {
            result = Tools.GetConversionResult(converter);
        }

        Tools.DestroyConverter(converter);

as in:

            ProcessingDocument = document;
            byte[] result = [];
            IntPtr converter = IntPtr.Zero;
            try
            {
                Tools.Load();
                converter = CreateConverter(document);
                Tools.SetPhaseChangedCallback(converter, OnPhaseChanged);
                Tools.SetProgressChangedCallback(converter, OnProgressChanged);
                Tools.SetFinishedCallback(converter, OnFinished);
                Tools.SetWarningCallback(converter, OnWarning);
                Tools.SetErrorCallback(converter, OnError);
                if (Tools.DoConversion(converter))
                {
                    result = Tools.GetConversionResult(converter);
                }
            }
            finally
            {
                if (converter != IntPtr.Zero)
                    Tools.DestroyConverter(converter);
            }

If anyone has any thoughts, I'd be happy to do a PR for this. or am I barking up the wrong tree?

[Shishkovskiy]

@simon-halsey-fadv It looks like you're barking up the right tree :) By using try-finally, you ensure that Tools.DestroyConverter(converter) is always called, even if an exception occurs during the conversion process. This helps prevent resource leaks if, for example, Tools.Load() or Tools.DoConversion(converter) throws an error. Probably there can be added a wrapper and implement the IDisposable to make it reusable or keep it as you've already suggested. At the same time, what if Tools.DestroyConverter(converter) fails? If Tools implement IDisposable, I believe it might be better to use using for Tools, as this ensures its disposal is attempted even if an exception occurs. However, since DestroyConverter is directly responsible for freeing unmanaged resources, probably there's a sense to handle exceptions somehow.