CODEWITHSUNDEEP
c#.netthread-safety
AbrahamJP
AbrahamJP

Reputation: 3430

Should I use a lock in the following scenario

I had designed a Stack wrapper class. My confusion is, should I be using lock while popping or pushing objects to the stack variable "ParameterStack". Please let me know, whether this class is thread safe or not. 

public static class StackManager
{
    private static Stack ParameterStack = new Stack();

    public static T Pop<T>()
    {
        T RawObject;
        T Result = default(T);

        lock (ParameterStack)
        {
            RawObject = (T)ParameterStack.Pop();
        }

        if (RawObject != null && RawObject is T)
            Result = (T)RawObject;

        return (T)Result;
    }

    public static void Push<T>(T Data)
    {
        lock (ParameterStack)
        {
            ParameterStack.Push(Data);
        }
    }
}

I had created this StackManager class for learning purpose.

Upvotes: 3

Views: 149

Answers (5)

Emad Omara
Emad Omara

Reputation: 532

Are you aware of the ConcurrentStack class? it is an efficient threadsafe implementation using lock-free

Upvotes: 0

oxilumin
oxilumin

Reputation: 4833

You should use ConcurrentStack in your case. If you are not able to use ConcurrentStack - you can use Stack.Synchronized() method:

Stack mySynchronizedStack = Stack.Synchronized(myStack);

But even if you use Synchronized() method enumerations still not thread safe, you should use a lock to enumerate your stack.

Stack myStack = new Stack();
lock (myStack.SyncRoot)
{
    foreach (var element in myStack)
    {
    }
}

Unfortunately, but the generic version of Stack do not have a Synchonization() method. So, you code should be:

public static class StackManager
{
    private static Stack ParameterStack;

    static StackManager()
    {
        ParameterStack = Stack.Synchronized(new Stack());
    }

    public static T Pop<T>()
    {
        object RawObject = ParameterStack.Pop();

        return RawObject is T ? (T)RawObject : default(T);
    }

    public static void Push<T>(T Data)
    {
        ParameterStack.Push(Data);
    }
}

You also should use object type for RawObject if you want to check the type. In your code you'll get an exception if you try to Pop object of different type.

Upvotes: 1

Henk Holterman
Henk Holterman

Reputation: 273229

It looks OK. There is a (rather theoretical) argument that locking on ParameterStack itself is not totally safe because you don't own the code. Suppose somewhere inside the Stack does a lock(this) , you could deadlock. 

public static class StackManager
{
    private static Stack parameterStack = new Stack();
    private static object stackLock = new object();

    // now use lock(stackLock) instead of lock(ParameterStack)

}

Upvotes: 7

Russell Troywest
Russell Troywest

Reputation: 8776

Yes, it's thread safe. As long as any use of the internal stack is within a lock it will be safe to use with multiple threads.

You could use a Stack<T> to avoid all the casting by the way. Or as oxilumin says use ConcurrentStack if you aren't just trying to learn how to make something threadsafe.

Upvotes: 1

Mihai Oprea
Mihai Oprea

Reputation: 2047

Yes, you should. Stack is not synchronized by default in C#.

Upvotes: 1

Related Questions