CODEWITHSUNDEEP
c#.netmultithreadingthread-safetylocking
Ashley Simpson
Ashley Simpson

Reputation: 383

Is this thread safe?

Is this thread safe?

private static bool close_thread_running = false;
public static void StartBrowserCleaning()
{
    lock (close_thread_running)
    {
        if (close_thread_running)
            return;

        close_thread_running = true;
    }

    Thread thread = new Thread(new ThreadStart(delegate()
    {
        while (true)
        {
            lock (close_thread_running)
            {
                if (!close_thread_running)
                    break;
            }

            CleanBrowsers();

            Thread.Sleep(5000);
        }
    }));

    thread.Start();
}

public static void StopBrowserCleaning()
{
    lock (close_thread_running)
    {
        close_thread_running = false;
    }
}

Upvotes: 4

Views: 712

Answers (1)

Jon Skeet
Jon Skeet

Reputation: 1500525

Well, it won't even compile, because you're trying to lock on a value type.

Introduce a separate lock variable of a reference type, e.g.

private static readonly object padlock = new object();

Aside from that:

If StopBrowserCleaning() is called while there is a cleaning thread (while it's sleeping), but then StartBrowserCleaning() is called again before the first thread notices that it's meant to shut down, you'll end up with two threads.

You might want to consider having two variables - one for "is there meant to be a cleaning thread" and one for "is there actually a cleaning thread."

Also, if you use a monitor with Wait/Pulse, or an EventHandle (e.g. ManualResetEvent) you can make your sleep a more reactive waiting time, where a request to stop will be handled more quickly.

Upvotes: 15

Related Questions