Threading problem.

  • Thread starter Thread starter muscha
  • Start date Start date
M

muscha

Hi All,

I'm having a bit of problem with threading, when I first call Start() on my
code the thread work, I call Stop() and it stopped, but when I call Start()
again my thread is not started. Although in the debugger I can see that it
went to the Start() method and creating the thread again.

Can anyone help me? This is the code:

<snip>

public static MySingleton GetInstance() {
if (instance == null) {
instance = new MySingleton();
}
return instance;
}

public void Start() {
lock (this) {
if (!started){
started = true;
thread = new Thread(new ThreadStart(Run));
thread.Start();
}
}
}

public void Stop() {
stopped = true;
}

private void Run() {
while (!stopped) {
Console.WriteLine("Running ...");
Thread.Sleep(1000);
}
thread.Join();
started = false;
}

</snip>


Thanks,

/m
 
muscha said:
I'm having a bit of problem with threading, when I first call Start() on my
code the thread work, I call Stop() and it stopped, but when I call Start()
again my thread is not started. Although in the debugger I can see that it
went to the Start() method and creating the thread again.
Click to expand...

Your code is slightly odd - I don't see why you're trying to call
thread.Join() within Run, when the thread it's running in should be the
thread it's trying to join anyway!

Note also that your singleton implementation isn't threadsafe - see
http://www.pobox.com/~skeet/csharp/singleton.html

Finally, the way you're testing the "stopped" flag is dodgy - because
you haven't got any synchronization there, there's no guarantee that
other threads will see new values.

If you could post a short but *complete* example which demonstrates the
problem, that would help.
 
Change the 2 methods below and it should work:

public void Stop()
{
lock (this)
{
stopped = true;
thread.Join();
started = false;
stopped = false;
}
}

private void Run()
{
while (!stopped)
{
Console.WriteLine("Running ...");
Thread.Sleep(1000);
}
}

José
 
Jon and all,
Note also that your singleton implementation isn't threadsafe - see
http://www.pobox.com/~skeet/csharp/singleton.html
Click to expand...

Thanks for that.
If you could post a short but *complete* example which demonstrates the
problem, that would help.
Click to expand...

Here is my class, I followed people's suggestion and it seems working now,
however is it thread safe?


using System;
using System.Threading;

namespace Scheduler
{
public class MySingleton
{
private static MySingleton instance;
private static Object padlock = new Object();
private Thread thread;
private bool stopped = false;
private bool started = false;

/// <summary>
/// Private constructor so this thread can't be instantiated.
/// </summary>
private MySingleton()
{
}

~MySingleton()
{
Stop();
}

public static MySingletonGetInstance()
{
if (instance == null)
{
lock (padlock)
{
if (instance == null)
{
instance = new MySingleton();
}
}
}
return instance;
}

public void Start()
{
lock (this)
{
if (!started)
{
started = true;
thread = new Thread(new ThreadStart(this.Run));
thread.Start();
}
}
}

public void Stop()
{
lock (this)
{
stopped = true;
thread.Join();
started = false;
stopped = false;
}
}

private void Run()
{
while (!stopped)
{
Console.WriteLine("Running ...");
Thread.Sleep(1000);
}
}
}
}

Thanks,

/m
 
Jon and all,
Note also that your singleton implementation isn't threadsafe - see
http://www.pobox.com/~skeet/csharp/singleton.html
Click to expand...

Thanks for that.
If you could post a short but *complete* example which demonstrates the
problem, that would help.
Click to expand...

Here is my class, I followed people's suggestion and it seems working now,
however is it thread safe?


using System;
using System.Threading;

namespace Scheduler
{
public class MySingleton
{
private static MySingleton instance;
private static Object padlock = new Object();
private Thread thread;
private bool stopped = false;
private bool started = false;

/// <summary>
/// Private constructor so this thread can't be instantiated.
/// </summary>
private MySingleton()
{
}

~MySingleton()
{
Stop();
}

public static MySingletonGetInstance()
{
if (instance == null)
{
lock (padlock)
{
if (instance == null)
{
instance = new MySingleton();
}
}
}
return instance;
}

public void Start()
{
lock (this)
{
if (!started)
{
started = true;
thread = new Thread(new ThreadStart(this.Run));
thread.Start();
}
}
}

public void Stop()
{
lock (this)
{
stopped = true;
thread.Join();
started = false;
stopped = false;
}
}

private void Run()
{
while (!stopped)
{
Console.WriteLine("Running ...");
Thread.Sleep(1000);
}
}
}
}

Thanks,

/m
 
Hi Muscha,

Looks to be thread safe after a quick glance anyway. It it were my code I
would first check that the thread is running in Stop to guard against the
possibility that Start was never called.

Cheers

Doug Forster
 
muscha said:
Jon and all,

Thanks for that.
Click to expand...

I note that you're using the double-checked locking algorithm though,
which I don't believe to be thread-safe.
Here is my class, I followed people's suggestion and it seems working now,
however is it thread safe?
Click to expand...

Not quite. Aside from the singleton problem, you're not locking on
anything before checking the stopped flag in Run. I would also suggest
not locking on "this" but locking on another reference private to the
class (just create a new object, like padlock but with an instance
variable).

I'm also not keen on the finalizer - do you think you really need it,
particularly? It's only going to be invoked when the AppDomain is
unloaded, and then only "some time after that". I would personaly just
get rid of it.
 
Here is my class, I followed people's suggestion and it seems working
now,
Not quite. Aside from the singleton problem, you're not locking on
anything before checking the stopped flag in Run. I would also suggest
not locking on "this" but locking on another reference private to the
class (just create a new object, like padlock but with an instance
variable).
Click to expand...

What's the difference from locking a separate object to lock than doing lock
on this? Isn't that by doing lock(this) in the beginning of the method, you
practically synchronize the entire method call?

thanks,

/m
 
Doug Forster said:
Looks to be thread safe after a quick glance anyway.
Click to expand...

And that's why you should never trust multithreading code to quick
glances :)
It it were my code I
would first check that the thread is running in Stop to guard against the
possibility that Start was never called.
Click to expand...

That would indeed be a good thing to do - I missed that.
 
muscha said:
What's the difference from locking a separate object to lock than doing lock
on this? Isn't that by doing lock(this) in the beginning of the method, you
practically synchronize the entire method call?
Click to expand...

If you lock on this, then something else might also lock on it, and you
could end up with a deadlock. If you lock on a reference that only your
instance will ever know about, you have more control.
 
Back
Top