Re: thread-safety
- From: RedLars <Liverpool1892@xxxxxxxxx>
- Date: Tue, 6 Jan 2009 05:43:44 -0800 (PST)
Thank you very much for such a detailed response. Will try and answer
your comments below.
On 6 Jan, 10:34, "Peter Duniho" <NpOeStPe...@xxxxxxxxxxxxxxxx> wrote:
On Tue, 06 Jan 2009 00:54:30 -0700, RedLars <Liverpool1...@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?
Apoligize for poor wording. What I'd like to achieve is for the Stop()
method to block until the consumer-thread actually has stopped
processing tasks. So if the job.Execute() is half-way into processing
a task, then I would like Stop to block until that task has been
completed before returning to calleer.
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).
The semantics I've worked by so far is Stop() means to pause-
processing-of-the-queue and not stop-adding-tasks-to-queue.
Say as an example I ran the following code;
ThreadClass.AddJob( job1 );
ThreadClass.AddJob( job2 );
ThreadClass.AddJob( job3 );
ThreadClass.Stop();
Assuming each job requires 1 second to complete, I would expect the
first job to be completed before the thread has been stopped
(obviously depends on current state of ThreadClass and timing).
So the way I see this, the while loop needs a "sleep" state, it cannot
rely on the queue.Next() to block because it might still contain data.
The Queue implementation uses ManualResetEvent.
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...
Point taken.
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.
Will look into that.
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.
The goal was to set the while loop into a non-processing-tasks-state.
Then the Start() method could interact with synchronization in
consumer thread.
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").
The point i was trying to make was, if the consumer thread is
currently processing a task when the Stop() is executed and Stop()
only changed the state, then method would return prematurely (before
the consumer thread actually was in a Stopped \ pause state).
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: Peter Duniho
- Re: thread-safety
- References:
- thread-safety
- From: RedLars
- Re: thread-safety
- From: Peter Duniho
- thread-safety
- Prev by Date: Re: How to pass a vc++ BSTR value to C#.Net
- Next by Date: Painting on a WebBrowser control
- Previous by thread: Re: thread-safety
- Next by thread: Re: thread-safety
- Index(es):
Relevant Pages
|
