lock not working in C#

Question

Lock is not working as expected, here is the code. I am applying thread here, but I will apply it to ASP.NET application.

class Program
    {
        static void Main(string[] args)
        {
            ThreadManager.CurrentSession = 0;
            for (int i = 0; i < 10; i++)
            {
                CreateWork objCreateWork = new CreateWork();
                ThreadStart start = new ThreadStart(objCreateWork.ProcessQuickPLan);
                new Thread(start).Start();
            }
            Console.ReadLine();
        }
    }

    class CreateWork
    {
        private object CurrentSession = -1;
        public void ProcessQuickPLan()
        {
            lock (CurrentSession)
            {
                CurrentSession = ThreadManager.CurrentSession;
                Console.WriteLine(CurrentSession);
                ThreadManager.CurrentSession = Convert.ToInt32(ThreadManager.CurrentSession) + 1;
            }
        }
    }

    class ThreadManager
    {
        public static object CurrentSession
        {
            get;
            set;
        }
    }

It is giving me following output

And I am expecting

Where am I doing wrong?

Should I use readonly object as described here C# lock(mylocker) not work

I can't use `readonly` objects, please help to find alternative if `readonly` is the solution — vikas, Aug 09 '13 at 10:31
Why can't you use `readonly` ?? That kind of artificial constraints is always weird. — H H, Aug 09 '13 at 10:35

H H · Answer 1 · 2013-08-09T10:45:24.450

The problem is in the object you use to lock on. You are using an instance variable so each instance has its own lock, that is fundamentally wrong.

The second issue is the initialization with -1, that is at least confusing.

The simple solution is a static object CurrentSession = new object();

The next issue is CurrentSession = ThreadManager.CurrentSession; . This makes no sense and is inherently wrong. I'm surprised it even compiles.

class CreateWork
{
    private object CurrentSession = -1;   // boxed int, Id only
    private static object _locker = new object();

    public void ProcessQuickPLan()
    {
        lock (_locker)
        {
            CurrentSession = ThreadManager.CurrentSession;
            Console.WriteLine(CurrentSession);
            ThreadManager.CurrentSession = Convert.ToInt32(ThreadManager.CurrentSession) + 1;
        }
    }
}

Summary: It's not clear what you're trying to do here. CurrentSession seems to have a double role as lock guard and Id. Not a good plan.

Basically you want 1 private static object to guard a resource. Never assign to it after initialization.

Sergey Kalinichenko · Accepted Answer · 2013-08-09T10:53:14.313

3

The problem is that each of your threads has its own lock. Making CurrentSession static should fix the problem: there will be only one object to lock on. You should also stop re-assigning it in your code.

class CreateWork
{
    private static readonly object LockObject = -1; // Although -1 works here, it's really misleading
    // You should consider replacing the above with a "plain" new object();
    private object CurrentSession = -1; 
    public void ProcessQuickPLan()
    {
        lock (LockObject)
        {
            CurrentSession = ThreadManager.CurrentSession;
            Console.WriteLine(CurrentSession);
            ThreadManager.CurrentSession = Convert.ToInt32(ThreadManager.CurrentSession) + 1;
        }
    }
}

Here is a working demo on ideone.

edited Aug 09 '13 at 10:53

answered Aug 09 '13 at 10:33

Sergey Kalinichenko

714,442
84
1,110
1,523

`CurrentSession` is static object – vikas Aug 09 '13 at 10:34
no it is not helpful, and even `ThreadManager.CurrentSession` is static – vikas Aug 09 '13 at 10:36
@WouterdeKort It does not matter what object you use - `-1` wrapped as an object will work just fine. – Sergey Kalinichenko Aug 09 '13 at 10:37
That's not true, see http://stackoverflow.com/questions/8267323/why-cant-we-lock-on-a-value-type – Wouter de Kort Aug 09 '13 at 10:39
1

@WouterdeKort That question has decidedly nothing to do with what the OP is doing: the fact that he's using a boxed `Int32` does not make `object` ineligible for locking. – Sergey Kalinichenko Aug 09 '13 at 10:40
@dasblinkenlight `private static object LockObject = -1;` this line is really helpful, and I am close to solve it. Thanks for your answer. Can you please elaborate that how lock works or can share some article. – vikas Aug 09 '13 at 10:43
thanks @dasblinkenlight, I accepted your answer, please share some article on this – vikas Aug 09 '13 at 10:48
You: _You should also stop re-assigning it_ Correct. Then, to be more sure it is not reassigned, why not use the `readonly` modifier on the field? – Jeppe Stig Nielsen Aug 09 '13 at 10:52
@JeppeStigNielsen That's a great suggestion, I made an edit. Thanks! – Sergey Kalinichenko Aug 09 '13 at 10:54
@vikas I don't know a good article that would explain this, but there is only one thing to check when the lock does not lock: "is this the same object?" In your case the answer was "no", because the code has been assigning new objects to the variable on which you lock. It is a good idea to make your lock object readonly. [MSDN Article](http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx) has a couple of important remarks on the subject, but that is all that you need to know in order to apply `lock` successfully. – Sergey Kalinichenko Aug 09 '13 at 10:59
thanks JeppeStigNielsen and dasblinkenlight for your great comments. and even for your great answers – vikas Aug 09 '13 at 11:02

score 0 · Answer 3 · answered Aug 09 '13 at 10:32

0

I think the problem is, that you lock an object in its own thread, so it will never really locked.

Better use global object, which will be locked.

answered Aug 09 '13 at 10:32

BendEg

20,098
17
57
131

I didn't get actually, please elaborate – vikas Aug 09 '13 at 10:33
@vikas You have `private object CurrentSession`. There's no `static` there. That means that each new `CreateWork` instance has its own `CurrentSession` object. So they are locking on different objects. To work, it must be the same instance they lock on. Make the field `static` (or "global"). – Jeppe Stig Nielsen Aug 09 '13 at 10:48
@JeppeStigNielsen, it means object should be static – vikas Aug 09 '13 at 10:51

score 0 · Answer 4 · answered Aug 09 '13 at 10:35

Each Thread contains its own instance of CreateWorkwith a locker. Try this code:

class Program
{
    static void Main(string[] args)
    {
        ThreadManager.CurrentSession = 0;
        CreateWork objCreateWork = new CreateWork();
        for (int i = 0; i < 10; i++)
        {
            ThreadStart start = new ThreadStart(objCreateWork.ProcessQuickPLan);
            new Thread(start).Start();
        }
        Console.ReadLine();
    }
}

class CreateWork
{
    private object CurrentSession = -1;
    public void ProcessQuickPLan()
    {
        lock (CurrentSession)
        {
            CurrentSession = ThreadManager.CurrentSession;
            Console.WriteLine(CurrentSession);
            ThreadManager.CurrentSession = Convert.ToInt32(ThreadManager.CurrentSession) + 1;
        }
    }
}

class ThreadManager
{
    public static object CurrentSession
    {
        get;
        set;
    }
}

score 0 · Answer 5 · answered Aug 09 '13 at 10:48

Change your code to this:

using System;
using System.Threading;
class Program
{
    static void Main(string[] args)
    {
        ThreadManager.CurrentSession = 0;
        for (int i = 0; i < 10; i++)
        {
            CreateWork objCreateWork = new CreateWork();
            ThreadStart start = new ThreadStart(objCreateWork.ProcessQuickPLan);
            new Thread(start).Start();
        }
        Console.ReadLine();
    }
}

class CreateWork
{
    private static object _lock = new Object();

    public void ProcessQuickPLan()
    {
        lock (_lock)
        {            
            Console.WriteLine(ThreadManager.CurrentSession);
            ThreadManager.CurrentSession++;
        }
    }
}

class ThreadManager
{
    public static int CurrentSession
    {
        get;
        set;
    }
}

The important thing is the separation between your lock and tracking your id.

The private lock is a static object so that it's shared between the threads. I've also removed assigning a new value to the lock each time.

lock not working in C#

5 Answers5