How to make static method thread safe?

Question

I have written a static class which is a repository of some functions which I am calling from different class.

public static class CommonStructures
{
    public struct SendMailParameters
    {
        public string To { get; set; }
        public string From { get; set; }
        public string Subject { get; set; }
        public string Body { get; set; }
        public string Attachment { get; set; }
    }
}

public static class CommonFunctions
{
    private static readonly object LockObj = new object();
    public static bool SendMail(SendMailParameters sendMailParam)
    {
        lock (LockObj)
        {
            try
            {
                //send mail
                return true;
            }
            catch (Exception ex)
            {
                //some exception handling
                return false;
            }
        }
    }

    private static readonly object LockObjCommonFunction2 = new object();
    public static int CommonFunction2(int i)
    {
        lock (LockObjCommonFunction2)
        {
            int returnValue = 0;
            try
            {
                //send operation
                return returnValue;
            }
            catch (Exception ex)
            {
                //some exception handling
                return returnValue;
            }
        }
    }
}

Question 1: For my second method CommonFunction2, do I use a new static lock i.e. LockObjCommonFunction2 in this example or can I reuse the same lock object LockObj defined at the begining of the function.

Question 2: Is there anything which might lead to threading related issues or can I improve the code to be safe thread.

Quesiton 3: Can there be any issues in passing common class instead of struct.. in this example SendMailParameters( which i make use of wrapping up all parameters, instead of having multiple parameters to the SendMail function)?

Regards, MH

Answer 1

Question 1: For my second method CommonFunction2, do I use a new static lock i.e. LockObjCommonFunction2 in this example or can I reuse the same lock object LockObj defined at the begining of the function.

If you want to synchronize these two methods, then you need to use the same lock for them. Example, if thread1 is accessing your Method1, and thread2 is accessing your Method2 and you want them to not concurrently access both insides, use the same lock. But, if you just want to restrict concurrent access to just either Method1 or 2, use different locks.

Question 2: Is there anything which might lead to threading related issues or can I improve the code to be safe thread.

Always remember that shared resources (eg. static variables, files) are not thread-safe since they are easily accessed by all threads, thus you need to apply any kind of synchronization (via locks, signals, mutex, etc).

Quesiton 3: Can there be any issues in passing common class instead of struct.. in this example SendMailParameters( which i make use of wrapping up all parameters, instead of having multiple parameters to the SendMail function)?

As long as you apply proper synchronizations, it would be thread-safe. For structs, look at this as a reference.

Bottomline is that you need to apply correct synchronizations for anything that in a shared memory. Also you should always take note of the scope the thread you are spawning and the state of the variables each method is using. Do they change the state or just depend on the internal state of the variable? Does the thread always create an object, although it's static/shared? If yes, then it should be thread-safe. Otherwise, if it just reuses that certain shared resource, then you should apply proper synchronization. And most of all, even without a shared resource, deadlocks could still happen, so remember the basic rules in C# to avoid deadlocks. P.S. thanks to Euphoric for sharing Eric Lippert's article.

But be careful with your synchronizations. As much as possible, limit their scopes to only where the shared resource is being modified. Because it could result to inconvenient bottlenecks to your application where performance will be greatly affected.

    static readonly object _lock = new object();
    static SomeClass sc = new SomeClass();
    static void workerMethod()
    {
        //assuming this method is called by multiple threads

        longProcessingMethod();

        modifySharedResource(sc);
    }

    static void modifySharedResource(SomeClass sc)
    {
        //do something
        lock (_lock)
        {
            //where sc is modified
        }
    }

    static void longProcessingMethod()
    {
        //a long process
    }

Answer 2

1) As to first it depends on what you want to have:

As is (two separate lock objects) - no two threads will execute the same method at the same time but they can execute different methods at the same time.

If you change to have single lock object then no two threads will execute those sections under shared locking object.

2) In your snippet there is nothing that strikes me as wrong - but there is not much of code. If your repository calls methods from itself then you can have a problem and there is a world of issues that you can run into :)

3) As to structs I would not use them. Use classes it is better/easier that way there is another bag of issues related with structs you just don't need those problems.

Answer 3

The number of lock objects to use depends on what kind of data you're trying to protect. If you have several variables that are read/updated on multiple threads, you should use a separate lock object for each independent variable. So if you have 10 variables that form 6 independent variable groups (as far as how you intend to read / write them), you should use 6 lock objects for best performance. (An independent variable is one that's read / written on multiple threads without affecting the value of other variables. If 2 variables must be read together for a given action, they're dependent on each other so they'd have to be locked together. I hope this is not too confusing.)

Locked regions should be as short as possible for maximum performance - every time you lock a region of code, no other thread can enter that region until the lock is released. If you have a number of independent variables but use too few lock objects, your performance will suffer because your locked regions will grow longer.

Having more lock objects allows for higher parallelism since each thread can read / write a different independent variable - threads will only have to wait on each other if they try to read / write variables that are dependent on each other (and thus are locked through the same lock object).

In your code you must be careful with your SendMailParameters input parameter - if this is a reference type (class, not struct) you must make sure that its properties are locked or that it isn't accessed on multiple threads. If it's a reference type, it's just a pointer and without locking inside its property getters / setters, multiple threads may attempt to read / write some properties of the same instance. If this happens, your SendMail() function may end up using a corrupted instance. It's not enough to simply have a lock inside SendMail() - properties and methods of SendMailParameters must be protected as well.

Answer 4

You can reuse the same lock object as many times as you like, but that means that none of the areas of code surrounded by that same lock can be accessed at the same time by various threads. So you need to plan accordingly, and carefully.

Sometimes it's better to use one lock object for multiple location, if there are multiple functions which edit the same array, for instance. Other times, more than one lock object is better, because even if one section of code is locked, the other can still run.

Multi-threaded coding is all about careful planning...

To be super duper safe, at the expense of potentially writing much slower code... you can add an accessor to your static class surround by a lock. That way you can make sure that none of the methods of that class will ever be called by two threads at the same time. It's pretty brute force, and definitely a 'no-no' for professionals. But if you're just getting familiar with how these things work, it's not a bad place to start learning.