1

I have a thread safe class:

internal class ExmplFile { private readonly string filename; private int resolution; private volatile PaddedImage gaussian; private object lockObject=new object(); //blah, blah, blah internal PaddedImage Gaussian() { if (gaussian != null) { return gaussian; } lock (lockObject) { if (gaussian == null) { Image(); if (File.Exists(filename + "-gaus.raw")) { gaussian = LoadImage(filename + "-gaus.raw", TerraGodContext.Instance() .Config.PpaCandidateRange); gaussian.ConformRepeatPadding(); } else { gaussian = new PaddedImage(resolution, resolution, false, TerraGodContext.Instance().Config.PpaCandidateRange); PaddedImage temp = new PaddedImage(resolution, resolution, false, TerraGodContext.Instance().Config.PpaCandidateRange); ImageProcessing.CalcGaussian(image,gaussian,temp, 16f* resolution /TerraGodContext.Instance().Config.ExmplResDS); } } } return gaussian; } }

Resharper gives me three warnings:

  1. On gaussian in gaussian.ConformRepeatPadding() it says Possible incorrect implementation of Double-check locking. Read access to checked field..
  2. On gaussian in gaussian = new PaddedImage( it says Possible incorrect implementation of Double-check locking. Possible multiple write access to checked field..
  3. On gaussian in ImageProcessing.CalcGaussian(image,gaussian,temp, it says Possible incorrect implementation of Double-check locking. Read access to checked field. (The same warning as number 1).

Am I being silly or Resharper is?

PS: In case I've missed something important, here is the full class code with parts I omitted above.

using System; using System.IO; using System.Text; namespace UPlus.TerrEngine { internal class ExmplFile : IEquatable<ExmplFile> { private readonly string filename; private int resolution; private volatile PaddedImage image; private volatile PaddedImage gaussian; private object lockObject=new object(); public ExmplFile(string abstractFileName, int res) { filename = new StringBuilder(abstractFileName).Append("-").Append(res.ToString("D4")).ToString(); resolution = res; } internal FilledList<MatchItem>[] GetMatchItems(int groupIdx, int regionIdx) { AlgorithmConfig Config = TerraGodContext.Instance().Config; if (resolution !=Config.ExmplResDS) throw new Exception(); PpaGraph exmplGraph = new PpaGraph(Config.ExmplResDS, Config.ExmplResDS, Config.ExmplResDS / Config.PpaExmplResDS); exmplGraph.Calculate(Image(), true, false, DebugOpts.BranchMin); exmplGraph.PrepareGraphForMatch(Config.ExmplCptRadius, Config.ExmplProcNodeDistance); exmplGraph.CalcExmplProcNodeGroups(groupIdx); MatchItemFinder matchItemFinder=new MatchItemFinder(Image(),Config.ExmplPatchSizeDS,true); matchItemFinder.Init(exmplGraph.processNodesGroups, regionIdx); return matchItemFinder.matchItems; } internal FastList<MatchItem> LoadMatchItems() { throw new Exception(); } internal PaddedImage Image() { if (image != null) { return image; } lock (lockObject) { if (image == null) { image=LoadImage(filename + ".raw", TerraGodContext.Instance().Config .PpaCandidateRange); } } return image; } internal PaddedImage Gaussian() { if (gaussian != null) { return gaussian; } lock (lockObject) { if (gaussian == null) { Image(); if (File.Exists(filename + "-gaus.raw")) { gaussian = LoadImage(filename + "-gaus.raw", TerraGodContext.Instance() .Config.PpaCandidateRange); gaussian.ConformRepeatPadding(); } else { gaussian = new PaddedImage(resolution, resolution, false, TerraGodContext.Instance().Config.PpaCandidateRange); PaddedImage temp = new PaddedImage(resolution, resolution, false, TerraGodContext.Instance().Config.PpaCandidateRange); ImageProcessing.CalcGaussian(image,gaussian,temp, 16f* resolution /TerraGodContext.Instance().Config.ExmplResDS); } } } return gaussian; } private PaddedImage LoadImage(string fileName, int padding) { PaddedImage img=new PaddedImage(resolution,resolution,false,padding); img.LoadRaw(fileName); img.ConformRepeatPadding(); return img; } public bool Equals(ExmplFile other) { return filename == other.filename; } public override int GetHashCode() { return filename.GetHashCode(); } } }

EDIT: Here is a screenshot in case the situation is not clear:

enter image description here

Improve this question
1
  • 1
    This is documented here and here Commented Jul 18, 2016 at 10:10

3 Answers 3

Reset to default
3

In general, within the lock, you should use local variables and make the write to the checked field the last possible action.

Otherwise, what are you doing to it in the middle? E.g. what does ConformRepeatPadding() do? I'm guessing it has side effects since it has no return value? But by the time you call it, you've already exposed that object by assigning it's value to the field.

Other threads may be able to observe this object in a "partially constructed" state if you assign to the field too early, so as I say, you should generally make the write to the field the last possible action inside the lock.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Yes ConformRepeatPadding() changes the state of gaussian. I didn't think of it that way since this method is the only way to get the gaussian field. Thanks to you now I get the reason and it makes sense.
Yeah, the local variable approach is valid, but I think Lazy<T> was designed to make bugs like this less likely to occur
2

Try defining your property using Lazy<T> instead. Your code looks correct, but that should get rid of the error and is a framework pattern for exactly this kind of thing

Improve this answer

1 Comment

Nice catch. Yes I will do that.
0

There is something strange with your lock. I've alwais seen it on static lock object and not private object. If your class isn't singleton, your lock alwais pass through as no lock was made.

Improve this answer

2 Comments

No it's not a singleton and I think using instance objects as lock monitor is completely valid .
yes, is valid until all concurrent thread use that instance for monitor concurred resource. if instance is thread dependend lock variable isn't shared and so no thread will wait any other thread

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.