CODEWITHSUNDEEP
c#asp.netlocking
vikas
vikas

Reputation: 2830

lock not working in C#

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

0
0
0
3
4
4
6
7
8
9

And I am expecting

0
1
2
3
4
5
6
7
8
9

Where am I doing wrong?

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

Upvotes: 3

Views: 3841

Answers (5)

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726569

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.

Upvotes: 4

Wouter de Kort
Wouter de Kort

Reputation: 39898

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.

Upvotes: 0

Henk Holterman
Henk Holterman

Reputation: 273244

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.

Upvotes: 4

margabit
margabit

Reputation: 2954

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;
    }
}

Upvotes: 0

BendEg
BendEg

Reputation: 21088

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.

Upvotes: 0

Related Questions