A Cool / Unexpected Refactoring around .Net C# Locking Issue

I’m constantly amazed by the insightfulness of ReSharper’s suggested refactorings (ReSharper is a Visual Studio Addin from JetBrains I use with C#). Today, I’ve been working on a threading problem where I’m getting crashes based on what seems like not proper locking across threads (they usually show up as some type of ugly update object or enum error).

My code starts like this:

 

public static List<DbProgressReport> DbProgressReportProperty { get; set; }
 

Then, I try wrapping the updates with a lock as follows (removing the set because I do it someplace else now)

 

public static List<DbProgressReport> DbProgressReportProperty
{
get
{
lock (lockDbProgressReportProperty)
{
return _dbProgressReportList;
}
}
}

 

I then realize that I need to copy the data because it may change while the caller prints it so I decide to return a temporary copy of the data as follows:

public static List<DbProgressReport> DbProgressReportProperty
{
get
{
lock (lockDbProgressReportProperty)
{
var dbProgressReportsNew = new List<DbProgressReport>();
if (_dbProgressReportList != null)
{
// make temp copy to avoid locking issues on read
foreach (var rec in _dbProgressReportList)
{
dbProgressReportsNew.Add(rec);
}
}
return dbProgressReportsNew;
}
}
}

I then notice that ReSharper has a suggestion.

image

Taking the suggestion, Resharper changes the code to:

lock (lockDbProgressReportProperty)
{
// make temp copy to avoid locking issues on
return _dbProgressReportList.ToList();
}
 
I expected a bunch of AddRange type stuff, but ReSharper figured out that ToList() would do everything I needed!
 
Very cool.
 
Hope this helps.
About Peter Kellner

Peter is a software professional specializing in mobile and web technologies. He has also been a Microsoft MVP for the past 7 years. To read more about Peter Kellner and his experience click here. For information about how Peter Kellner might be able to help you with your project click here.

Follow me:


Comments

  1. Hi! Someone in my Myspace group shared this website with us so I came to take a look. I’m definitely loving the information. I’m book-marking and will be tweeting this to my followers! Fantastic blog and excellent design.

  2. I like ReSharper a lot too. I’ve rarely found it’s refactorings to be off. If nothing else, it’s always good to have something making the suggestion so you can consider all angles!

  3. Jelle,
    I think you found a cut and paste error on my part. The “if” was not actually their when I did the refactor.

  4. Did Resharper catch that you always assigned _dbProgressReportList? Otherwise it removed the “if (_dbProgressReportList == null)” where it should not have, the code will then behave differently after the refactoring.

  5. James Curran says:

    Hmmm…. I would probably have written it as:

    return new List(_dbProgrssReportList);

    But I guess that’s probably better….

Follow

Get every new post delivered to your Inbox

Join other followers: