Is this extension method thread safe?

Question

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.

Answer 1

I did a simple testing of extension method with a class (reference type), the result looks good (thread safe).

Main Program

 static void Main(string[] args)
    {
        List<Car> cars = new List<Car>();

        cars.Add(new Car() { Brand = "Alfa Romeo", PlateNo = "A1" });
        cars.Add(new Car() { Brand = "BMW", PlateNo = "B1" });
        cars.Add(new Car() { Brand = "Honda", PlateNo = "H1" });
        cars.Add(new Car() { Brand = "Mercedes", PlateNo = "M1" });
        cars.Add(new Car() { Brand = "Renault", PlateNo = "R1" });
        cars.Add(new Car() { Brand = "Toyota", PlateNo = "T1" });

        Parallel.ForEach(cars, car =>
        {
            new ParallelOptions { MaxDegreeOfParallelism = 2 };
            
            Console.WriteLine($"Step 1, thread id {Thread.CurrentThread.ManagedThreadId}: {car.Brand} - {car.PlateNo}");
            
            car.ChangePlateNo();
            
            Console.WriteLine($"Step 4, thread id {Thread.CurrentThread.ManagedThreadId}: {car.Brand} - {car.PlateNo}");

        });

        Console.WriteLine("Press any key to close.");
    }

Car Class

public class Car
{
    public string Brand { get; set; }
    public string PlateNo { get; set; }
}

Extension method

public static class Extension
{
    public static void ChangePlateNo(this Car car)
    {
        Console.WriteLine($"Step 2, thread id {Thread.CurrentThread.ManagedThreadId}: {car.Brand} - {car.PlateNo}");

        // let a record sleep 3 seconds
        if (car.Brand == "BMW")
            System.Threading.Thread.Sleep(3000);

        car.PlateNo = car.PlateNo + "2";


        Console.WriteLine($"Step 3, thread id {Thread.CurrentThread.ManagedThreadId}: {car.Brand} - {car.PlateNo}");
    }
}

Output

Step 1, thread id 1: Alfa Romeo - A1
Step 1, thread id 4: Honda - H1
Step 1, thread id 3: BMW - B1
Step 1, thread id 5: Mercedes - M1
Step 1, thread id 7: Toyota - T1
Step 1, thread id 6: Renault - R1
Step 2, thread id 6: Renault - R1
Step 3, thread id 6: Renault - R12
Step 4, thread id 6: Renault - R12
Step 2, thread id 1: Alfa Romeo - A1
Step 3, thread id 1: Alfa Romeo - A12
Step 4, thread id 1: Alfa Romeo - A12
Step 2, thread id 4: Honda - H1
Step 3, thread id 4: Honda - H12
Step 4, thread id 4: Honda - H12
Step 2, thread id 7: Toyota - T1
Step 3, thread id 7: Toyota - T12
Step 4, thread id 7: Toyota - T12
Step 2, thread id 3: BMW - B1
Step 2, thread id 5: Mercedes - M1
Step 3, thread id 5: Mercedes - M12
Step 4, thread id 5: Mercedes - M12
Step 3, thread id 3: BMW - B12
Step 4, thread id 3: BMW - B12

Answer 2

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

Answer 3

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.

Answer 4

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.