Possible false positive memory leak reports in tests

[br3aker]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have verified that I am running the latest version of ImageSharp
• [X] I have verified if the problem exist in both DEBUG and RELEASE mode
• [X] I have searched open and closed issues to ensure it has not already been reported

ImageSharp version

main branch

Other ImageSharp packages and versions

None

Environment (Operating system, version and so on)

Windows 10

.NET Framework version

net6

Description

We have TestFile class for testing purposes. It works as a cache of some sort and has this line:

https://github.com/SixLabors/ImageSharp/blob/2c42e41ccffe7bf5123cdc0746707c07ca0e5523/tests/ImageSharp.Tests/TestFile.cs#L71

I think this is responsible for random false positive memory leaks in tests, e.g. what happens in this case if memory leaks diagnostics are on and if we would imagine this is the only test in out test suite?

// TestFile instance is injected by test attributes
// Doesn't really matter in this example
[Theory]
public void Test(TestFile testFile)
{
    // testFile.CreateRgba32Image() creates 2 Image<Rgba32> objects
    // one is lazily initialized and stored in cache and one is created via Clone() call
    // and used in this test
    using (var image = testFile.CreateRgba32Image())
    {
        // Undisposed objects count should be 2 here
        this.Output.WriteLine(image.Size());
    }
    // Cloned image is disposed and TestFile instance image is not disposed
    // Thus undisposed count should be 1 here which can lead to false positive memory leak
}

And aren't these lines in TestFile.cs:

public byte[] Bytes => this.bytes ?? (this.bytes = File.ReadAllBytes(this.FullPath));

private Image<Rgba32> Image => this.image ?? (this.image = ImageSharp.Image.Load<Rgba32>(this.Bytes));

not thread safe?

File.ReadAllBytes(this.FullPath) can be called by multiple threads and won't leak as it returns managed array. this.image = ImageSharp.Image.Load<Rgba32>(this.Bytes) can also be called by multiple threads but extra instance would just leak.

Steps to Reproduce

This happens quite randomly so there's no 100% correct steps to reproduce it.

Images

No response

[JimBobSquarePants]

I think you are onto something here. The concurrent dictionary does not protect us in this case.

[JimBobSquarePants]

I wonder if we can simply get away with sticking a lock round it?

[br3aker]

Yes I believe but it can downgrade test performance. Situation when two threads access same image reference which would be null should be fairly rare to use a lock here. Maybe we can use something like this?

private Image<Rgba32> image;
private Image<Rgba32> Image
{
    get 
    {
        var img = Interlocked.Read(this.image);
        if (img is null)
        {
            var loadedImg = ImageSharp.Image.Load<Rgba32>(this.Bytes);
            img = Interlocked.CompareExchange(location1: this.image, value: loadedImg, comparand: null);
            if (img is not null)
            {
                loadedImg.Dispose();
            }
            else
            {
                img = loadedImg;
            }
        }
        
        return img;
    }
}

Yes, this still allows same image to be loaded multiple times but we don't introduce a lock for every test which uses this image. Lock doesn't really cost much if nobody actually waits on it but problem with lock is that we need to lock both read and write operations which concerns me a bit.

[JimBobSquarePants]

Yeah, I totally overlooked the read costs. hat looks solid to me. If I get the chance later I'll use it in a PR.

[JimBobSquarePants]

Was going to have a look at this but realized var img = Interlocked.Read(this.image); doesn't compile.

[br3aker]

Was going to have a look at this but realized var img = Interlocked.Read(this.image); doesn't compile.

Hi! Well, I was sure it existed :P I guess we can just use private volatile Image<Rgba32> image; and 'normal' load instead:

private volatile Image<Rgba32> image;
private Image<Rgba32> Image
{
    get 
    {
        var img = this.image;
        if (img is null)
        {
            var loadedImg = ImageSharp.Image.Load<Rgba32>(this.Bytes);
            img = Interlocked.CompareExchange(location1: this.image, value: loadedImg, comparand: null);
            if (img is not null)
            {
                loadedImg.Dispose();
            }
            else
            {
                img = loadedImg;
            }
        }
        
        return img;
    }
}

Whole idea with Interlocked.Read was about cache coherency which even if failed would be corrected by Interlocked.CompareExchange line.