Request review of this simple threading code

[sklett]

In the past I've had problems with using threads and getting all kinds of
weird bugs.
I've been reading over Jon Skeet's GREAT website about threading and have
tried to apply some of what I learned. There is almost too much information
there for me, I had a hard time deciding what method or design to choose.

I hoped that I could post this skeleton class for a quick review to make
sure I'm on the right track. If you have a minute, I would appreciate your
comments.

public class CalibrateDAQViewPresenter :
FunctionalTestModulePresenter<ICalibrateDAQView>
{
private bool _stopping = false;
private bool _stopped = false;
private static readonly object _lockObject = new object();
 
protected override void Dispose(bool disposing)
{
// let the polling thread exit
lock (_lockObject)
{
_stopping = false;
while (_stopped == false) ;
}
base.Dispose(disposing);
}
 
public void OnViewReady()
{
_startReadingData();
}
 
private void _startReadingData()
{
_stopped = false;
_stopping = false;
ThreadPool.QueueUserWorkItem(
delegate
{
_dataPoller();
}
);
}
 
private void _dataPoller()
{
while(true)
{
lock(_lockObject)
{
if(_stopping)
{
_stopped = true;
return;
}
// do stuff and update view
}
 
Thread.Sleep(100);
}
}
}
 

Thanks for reading!

[Adam Clauss]

Your code will deadlock upon Dispose. Dispose locks on _lockObject, but
does not release that lock until the thread has stopped. The thread on the
otherhand, requires that same lock in order to stop.

--
Adam Clauss

"sklett" <[e-mail] in message
news:e4**************@TK2MSFTNGP05.phx.gbl...

In the past I've had problems with using threads and getting all kinds of
weird bugs.
I've been reading over Jon Skeet's GREAT website about threading and have
tried to apply some of what I learned. There is almost too much
information there for me, I had a hard time deciding what method or design
to choose.

I hoped that I could post this skeleton class for a quick review to make
sure I'm on the right track. If you have a minute, I would appreciate
your comments.

public class CalibrateDAQViewPresenter :
FunctionalTestModulePresenter<ICalibrateDAQView>
{
   private bool _stopping = false;
   private bool _stopped = false;
   private static readonly object _lockObject = new object();

   protected override void Dispose(bool disposing)
   {
       // let the polling thread exit
       lock (_lockObject)
       {
           _stopping = false;
           while (_stopped == false) ;
        }
       base.Dispose(disposing);
   }

   public void OnViewReady()
   {
       _startReadingData();
   }

   private void _startReadingData()
   {
       _stopped = false;
       _stopping = false;
       ThreadPool.QueueUserWorkItem(
           delegate
           {
               _dataPoller();
           }
       );
   }

   private void _dataPoller()
   {
       while(true)
       {
           lock(_lockObject)
          {
                if(_stopping)
               {
                    _stopped = true;
                    return;
                }
                // do stuff and update view
           }

           Thread.Sleep(100);
         }
    }
}
 

Thanks for reading!

[Adam Clauss]

"Adam Clauss" <ca*****@tamu.eduwrote in message
news:12*************@corp.supernews.com...

Your code will deadlock upon Dispose. Dispose locks on _lockObject, but
does not release that lock until the thread has stopped. The thread on
the otherhand, requires that same lock in order to stop.

One solution to this - get rid of the stopped flag.
In Dispose, replace the while loop (waiting on stopped) with a call to:
Monitor.Wait(_lockObject)

That will actually release the lock (allowing the thread to acquire it).
Then, in _dataPoller(), replace the:
_stopped = true;
with a call to:
Monitor.Pulse(_lockObject);

Note: this assumes you are not elsewhere using the _stopped flag.
Consideration may have to be added if other's need this as well.

--
Adam Clauss

[Marc Gravell]

a: What bugs / errors? This doesn't actually demonstrate anything

b: you're using a static lock at the instance level, so sharing a lock
between all instances; probably not what you wanted

c: if this is talking to a WinForm UI you will need to .Invoke/.BeginInvoke
to the UI to get the UI updates to work

d: while (_stopped == false) ;

This might never exit, as _stopped isn't volatile, so it will probably be
cached in a register; secondly, it is hugely intensive; personally I'd do
something more like the following:

lock(_lockObject) {
while(!_stopped) {
Monitor.Wait(_lockObject);
}
}

And "_stopped = true;" would become:
_stopped = true;
Monitor.Pulse(_lockObject);

Marc

[Marc Gravell]

Oh, and re the subject "of this simple threading code"... I'm not sure that
such exists in this universe ;-p

[sklett]

Thank you for the help, Adam. I see the problems with the code and am
reworking it now to take your suggestions into consideration. I think I
might move the "thread stopping" logic out of the dispose method and handle
it elsewhere, possibly the Closing event.

thanks again,
Steve
"Adam Clauss" <ca*****@tamu.eduwrote in message
news:12*************@corp.supernews.com...

"Adam Clauss" <ca*****@tamu.eduwrote in message
news:12*************@corp.supernews.com...

>Your code will deadlock upon Dispose. Dispose locks on _lockObject, but
does not release that lock until the thread has stopped. The thread on
the otherhand, requires that same lock in order to stop.

One solution to this - get rid of the stopped flag.
In Dispose, replace the while loop (waiting on stopped) with a call to:
Monitor.Wait(_lockObject)

That will actually release the lock (allowing the thread to acquire it).
Then, in _dataPoller(), replace the:
_stopped = true;
with a call to:
Monitor.Pulse(_lockObject);

Note: this assumes you are not elsewhere using the _stopped flag.
Consideration may have to be added if other's need this as well.

--
Adam Clauss

[sklett]

Thanks for the tips Marc.

You and Adam have suggested similar solutions, that's always a good thing
;0)

Have a good one,
Steve
"Marc Gravell" <ma**********@gmail.comwrote in message
news:OR**************@TK2MSFTNGP03.phx.gbl...

a: What bugs / errors? This doesn't actually demonstrate anything

b: you're using a static lock at the instance level, so sharing a lock
between all instances; probably not what you wanted

c: if this is talking to a WinForm UI you will need to
.Invoke/.BeginInvoke to the UI to get the UI updates to work

d: while (_stopped == false) ;

This might never exit, as _stopped isn't volatile, so it will probably be
cached in a register; secondly, it is hugely intensive; personally I'd do
something more like the following:

lock(_lockObject) {
while(!_stopped) {
Monitor.Wait(_lockObject);
}
}

And "_stopped = true;" would become:
_stopped = true;
Monitor.Pulse(_lockObject);

Marc