Re: thread-safety
- From: "Peter Duniho" <NpOeStPeAdM@xxxxxxxxxxxxxxxx>
- Date: Tue, 06 Jan 2009 02:34:49 -0700
On Tue, 06 Jan 2009 00:54:30 -0700, RedLars <Liverpool1892@xxxxxxxxx> wrote:
[...]
This setup has worked fine so far. Now I need to implement a stop()
method which brings up a few concerns regarding thread safety.
Just for the record, there's nothing in the code you posted that suggests it's itself thread-safe. We might assume it is, but that would depend on the implementation of your Queue class (obviously not one of the .NET queue classes).
A few comments
* A job.Execute() beeing executed must be completed before the thread
is stopped.
* A job.Execute() operation might take upto a few seconds to complete
(Asynch operation).
* Any unprocessed tasks in the queue, or created timers shall not be
deleted by Stop() method.
* The Queue (queue variable) has a Wakeup() method that stops the
queue.Next() from blocking and returns null;
To prevent stop() occuring during execution of a job a lock(..) was
introduced.
What do you mean "prevent stop() occurring during execution of a job"? As you've already noted, until your job.Execute() method comes back, your consumer thread isn't going to do anything else, including stopping. What's in the "stop()" method that you need to not occur until after job.Execute() returns?
private object threadLocker = new object();
private void Thread_Main() {
while (!TerminatedThread) {
lock (threadLocker) {
IJob job = queue.Next() as IJob;
if (job != null) job.Execute();
}
}
}
What other code locks on "threadLocker"? Without knowing that, there's no useful comments to be made about the above.
To actually stop the while loop from running I thought about adding
Monitor.Wait(...) to the loop.
How would that be different from your queue implementation which already has a "release on new item or wake-up" semantic built right in? (For that matter, I'm surprised at the implication that the queue implementation doesn't already use Monitor.Wait() or something similar).
But to be able to add such mechanism I
would need a state flag to distinguish between running and not running
(stopped) mode. So I thought about adding private enum State
{ Created, Started, Stopped };
private volatile State state;
private ManualResetEvent StopThreadResetEvent = new ManualResetEvent
(false);
private void Thread_Main()
{
while (!TerminatedThread) {
if (state == State.Started) {
lock (threadLocker) {
IJob job = queue.Next() as IJob;
if (job != null) job.Execute();
}
}
else if (state == State.Stopped)
{
Monitor.Wait(StopThreadResetEvent);
}
}
That code won't work. The thread has to have taken the lock on "StopThreadResetEvent" before it can call Monitor.Wait() using that object. That said...
Who sets "StopThreadResetEvent"? Why do you need that? The Monitor class certainly doesn't. It's odd for you to be passing a WaitHandle of any sort to the Monitor class; if you've got a WaitHandle, you would normally just wait on _that_, rather than using the WaitHandle simply as a synchronization object for Monitor.
So would it be enough to implement the stop method like this (?);
private void Stop() {
if (state == State.Started) {
state = State.Stopped;
queue.Wakeup();
}
}
This Stop() method doesn't interact with any of the synchronization you put into your consumer thread. If you meant for the synchronization to somehow affect the way that the Stop() method works, you've failed in that goal.
From what I can understand this would not guarantee that Thread_Main
is at Monitor.Wait(...) when Stop() returns.
Could that be a problem?
It would be a problem to call Monitor.Pulse() if no other thread is waiting at a call to Monitor.Wait(), because then the waiting thread would miss the Pulse(). However, typically the waiting thread would always be either in the lock or waiting, ensuring that no other thread _can_ call Monitor.Pulse() until the waiting thread is in fact waiting.
I guess I could add
lock (threadLocker) { /* no code */ }
after Wakeup() as a form of waiting until Thread_Main has completed
its current task.
I don't see what the point of that would be. It would only block if some other thread already had the "threadLocker" lock, but a) there's nothing in the code you posted that would indicate your consumer thread would be doing anything useful with that lock after you lock on it, and b) if that thread _were_ going to do something useful with that lock, you would do that useful thing _inside_ the locked block of code (instead of "no code").
I would also need to change the Start() method a bit;
public void Start() {
if (state == State.Created)
thread = new Thread(new ThreadStart(this.Thread_Main));
thread.Start();
}
else if (state == State.Stopped)
{
state = State.Started
StopThreadResetEvent.Set();
}
}
First, you need synchronization around the code that affects the "state" variable. Second, here you actually use the ManualResetEvent in the usual way (by setting it), but setting it will have no effect at all on the call to Monitor.Wait() in your consumer thread.
You seem to be trying to mix and match different thread synchronization techniques. That's not going to work.
What if Stop() and Start() are executed within a very short timeframe
- could that cause a problem with the current code?
All due respect, the code you posted already has so many problems that a theoretical race condition is the least of your worries. :)
That said, if you implement everything correctly, there should be no problem at all with the timing of the calls to Stop() and Start().
I get the feeling I'm missing something when it comes handling state
\state-changes in a multi-threaded environment.
Appreicate any comments.
There's nothing particularly difficult about dealing with "state-changes", but you do need to use the thread synchronization mechanisms correctly.
I can't offer much else other than the above comments, without a concise-but-complete code sample that accurately illustrates your situation. The fundamental idea seems sound (you want a consumer thread that you can essentially pause, and which also already has a "wait" semantic in it to deal with queue insertions), but you seem to be over-complicating things.
Part of the problem seems to me to be that your queue implementation itself has some synchronization mechanism built into it, which means you don't have good control over the way the Next() method operates. Even so, since your queue does have the "wake-up" method, it seems to me that you should be able to take advantage of that rather than adding a new WaitHandle into the mix (your ManualResetEvent object).
And if you for some reason decide that you _must_ use the WaitHandle semantics in addition to the queue's existing semantics, you obviously should be sure to use it correctly. When using a WaitHandle, the Monitor class is superfluous and won't work the way you seem to be trying to use it.
Assuming the queue implementation really does already have correct thread synchronization built into it, then I suspect you might not need to add any synchronization at all to your consumer thread. You should be able to make your state variable "volatile", and then just use the synchronization that's implicit in your queue. If you find that you must do the "check value then set" with your state variable, then you'll have to synchronize that (e.g. with a "lock" statement), so that multiple threads don't wind up doubling up on operations or failing to complete an intended operation.
Pete
.
- Follow-Ups:
- Re: thread-safety
- From: RedLars
- Re: thread-safety
- References:
- thread-safety
- From: RedLars
- thread-safety
- Prev by Date: Tran Hung and Ha Thuy Nga Pagis by night
- Next by Date: Re: Populate an array with random numbers
- Previous by thread: thread-safety
- Next by thread: Re: thread-safety
- Index(es):
Relevant Pages
|
