2

I have created extension methods to map between objects but I am worried it may not be thread safe. Here is the method:

public static SavableRecord ToSavableRecordForMongoDB(this Record record)
{
    SavableRecord savableRecord = new SavableRecord();

    if (record.Fields == null)
        throw new ArgumentException("Fields of record cannot be null");

    if (string.IsNullOrWhiteSpace(record.id))
        savableRecord._id = record.id;

    foreach (KeyValuePair<string, Field> item in record.Fields)
        savableRecord.Fields[item.Key] = new Field(item.Value);

    return savableRecord;
}

If this method is not thread safe what can I do to make it thread safe.

Update

The record object is passed in the controller in an MVC project. The record object is never changed while in this controller or its path.

Community
  • 1
  • 1
Luke101
  • 59,479
  • 80
  • 216
  • 348
  • 1
    This all depends on whether other threads will be using the `Record` passed to this extension method. – aevitas Sep 19 '15 at 18:52
  • I see, so if other threads change the value of the record object then those value may possibly change in this extension method. Other threads will not use the record object so I should be good? – Luke101 Sep 19 '15 at 18:56
  • Theoretically, yes. But again, it's all down to implementation. Check your code paths and make sure that no race conditions can occur. On a side note, that `ArgumentException` should probably be an `ArgumentNullException`. – aevitas Sep 19 '15 at 18:58

3 Answers3

8

That depends on whether Record is immutable or not.

If the Record's id and Fields are only set during object creation, than this method is pure and thread-safe - it creates no side effects (assuming SavableRecord doesn't) and will always return the same result for the same input. You can call it in parallel on ten threads without any conflict, data corruption or confusion.

However, if Fields (or id) are mutable, one thread can change the value while this method executes, leading to unexpected results, starting from the Fields iterator throwing an exception, properties getting modified to invalid values after your validation checks, and down to values getting missed, or two calls to this function returning different values.

Avner Shahar-Kashtan
  • 14,140
  • 3
  • 37
  • 61
  • *Immutable* — good point. Updated my own answer with a reference to your's. – Ondrej Tucny Sep 19 '15 at 18:59
  • Hi @Avner Shahar-Kashtan, question. The class extension,ToSavableRecordForMongoDB, is static. Couldn't multiple threads be accessing/using this static method at the same time? Wouldn't this means both threads would be attempting to modify Record at the same time and therefore a race condition would exist? – Fractal Aug 08 '19 at 04:33
  • 1
    @Fractal you can have 5 threads or 50. As long as each thread only *reads* from the Record and creates a different, distinct SavableRecord, they can coexist safely. – Avner Shahar-Kashtan Aug 08 '19 at 04:50
  • okay, thanks so much @Avner Shahar-Kashtan for the clarification. makes sense. – Fractal Aug 08 '19 at 04:54
6

No, in general this method is definitely not thread-safe in respect to its argument record. How can you tell? You are accessing multiple properties of the same object at (necessarily) different times—all without being contained within one synchronization section (e.g. a lock statement). Hence any other thread may change the object in the meantime, rendering it inconsistent in respect to what the logic of your method expects.

The most serious example is a null check of record.Fields at the beginning, plus accessing the same property in a loop later in the same method. In case any other thread assigns null to record.Fields in the meantime, your code will necessarily cause a NullReferenceException. Plus this behavior will, most likely, look like being completely random.

However, in case the object was immutable as Avner's answer suggests, the situation would be different and your code would be thread-safe.

If this method is not thread safe what can I do to make it thread safe.

Use a lock statement:

public static SavableRecord ToSavableRecordForMongoDB(this Record record)
{
    lock (lockObject)
    {
        … the logic of your method …
    }
}

What to assume for lockObject highly depends on the overall logic of your code. As a matter of course, all threads that need write-access to the same Record instance need to synchronize on the same lockObject in order to achieve thread-safe behavior. For more information regarding various types of thread synchronization see e.g. this MSDN article.

Community
  • 1
  • 1
Ondrej Tucny
  • 26,707
  • 6
  • 66
  • 88
  • 1
    Using a `lock` won't really help here unless you can ensure any other code that modifies the record also checks the same lock. This won't prevent a different method from modifying the item's properties. – Avner Shahar-Kashtan Sep 19 '15 at 19:03
  • 1
    @AvnerShahar-Kashtan I implied that. But generally a good point for improvement and an explicit mention. – Ondrej Tucny Sep 19 '15 at 19:04
1

Guess same principles that apply to an instance method thread safety applies to extension methods as well. In the above case every thread that accesses the ToSavableRecordForMongoDB method will be getting new instance of the class in which the method is declared. And in the method as such i do not see any static fields being modified. Looks thread safe to me

Raghavan MK
  • 11
  • 3