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

Make TestFile Image threadsafe. #2225

Merged
merged 3 commits into from
Sep 14, 2022
Merged

Make TestFile Image threadsafe. #2225

merged 3 commits into from
Sep 14, 2022

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Sep 12, 2022

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fix #2178

CC @br3aker

Image<Rgba32> img = this.image;
if (img is null)
{
Image<Rgba32> loadedImg = ImageSharp.Image.Load<Rgba32>(this.Bytes);
Copy link
Contributor

@br3aker br3aker Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can do a double check lock instead so image would never be loaded twice:

// this.image should be marked volatile still
if (this.image is null)
{
    lock (this.lockObject)
    {
        if (this.image is null)
        {
            this.image = ImageSharp.Image.Load<Rgba32>(this.Bytes);
        }
    }
}
return this.image;

I proposed Interlocked solution, I know, but lock solution is a lot more readable IMO and doesn't have a possible flaw of loading the same image twice. It's also isn't slow on reads due to locking as most of the time first if check would be true.

Maybe we should use it instead due to simplicity?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely far more readable. I’m happy to switch out

/// <summary>
/// Used to ensure image loading is threadsafe.
/// </summary>
private readonly object syncLock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private readonly object syncLock;
private readonly object syncLock = new();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! Can't believe I missed that!

@JimBobSquarePants JimBobSquarePants merged commit 5f796cd into main Sep 14, 2022
@JimBobSquarePants JimBobSquarePants deleted the defect/2718 branch September 14, 2022 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible false positive memory leak reports in tests
3 participants