Is this a BackgroundWorker?

[Harlan Messinger]

A few years ago I created an interface, IMonitoredProcess:

public interface IMonitoredProcess
{
event ProgressUpdateHandler ProgressUpdated;
event ProcessStatusHandler ProcessStatusUpdated;
event ProcessEndedHandler Ended;
void Run();
void Cancel(object source, EventArgs e);
int ItemCount { get ; }
}

I created an abstract class, AbstractFileProcessor, that implements
IMonitoredProcess, as a base class for processes that, given a list of
relative file paths, would iterate through them and conduct some
operation on each file with respect to one or two locations where the
files are located (checking that they exist, comparing them between two
locations, copying them from one place to another). The
interface-implementing code in AbstractFileProcessor is:

public event ProgressUpdateHandler ProgressUpdated;
protected void RaiseProgressUpdated(object source,
ProgressUpdateEventArgs e)
{ if (ProgressUpdated != null) ProgressUpdated(source, e); }

public event ProcessStatusHandler ProcessStatusUpdated;
protected void RaiseProcessStatusUpdated(object source,
ProcessStatusEventArgs e)
{
if (ProcessStatusUpdated != null)
ProcessStatusUpdated(source, e);
}

public event ProcessEndedHandler Ended;
protected void RaiseEnded(object source, ProcessEndedEventArgs e)
{if (Ended != null) Ended(source, e); }

public virtual void Cancel(object source, EventArgs e)
{
canceled = true;
}

public virtual void Run()
{
int itemNumber = 0;
ProcessEndType endType = ProcessEndType.Normal;
try
{
foreach (string subpath in fileList)
{
itemNumber++;
RaiseProgressUpdated(
this, new ProgressUpdateEventArgs(this,
ProcessStatus.Running, itemNumber, subpath));
ProcessOneFile(subpath);
if (canceled) break;
}
}
catch(Exception ex)
{
endType = ProcessEndType.Aborted;
RaiseProcessStatusUpdated(
this, new ProcessStatusEventArgs(this,
ProcessStatus.Running, ex.ToString()));
}
finally
{
if (canceled)
endType = ProcessEndType.Canceled;
RaiseEnded(this, new ProcessEndedEventArgs(this, endType));
}
}

Finally, the calling app instantiates one of the classes derived from
AbstractFileProcessor, attaches its Run method to a thread, and kicks
off the thread:

Thread thread = new Thread(new ThreadStart(process.Run));
thread.Start();


I only recently came across the BackgroundWorker class. Now I'm thinking
that I could add to my AbstractFileProcessor class a method that creates
its own thread and hands the object's own Run method to it,

public virtual void Launch()
{
Thread thread = new Thread(new ThreadStart(this.Run));
thread.Start();
}

(and I could even make the Run method private or protected), and then
the application could call Launch directly without needing to create a
thread first--and if I did that, then my AbstractFileProcessor would be
more or less the same thing as a BackgroundWorker. Is that correct?

 

[Peter Duniho]

[...]
I only recently came across the BackgroundWorker class. Now I'm thinking
that I could add to my AbstractFileProcessor class a method that creates
its own thread and hands the object's own Run method to it,

public virtual void Launch()
{
Thread thread = new Thread(new ThreadStart(this.Run));
thread.Start();
}

(and I could even make the Run method private or protected), and then
the application could call Launch directly without needing to create a
thread first--and if I did that, then my AbstractFileProcessor would be
more or less the same thing as a BackgroundWorker. Is that correct?

That depends entirely on your definition of "more or less". The class
you posted isn't even thread-safe (among other things, I sure hope you
have no client code modifying event subscriptions once you start the
thread executing the Run() method), never mind does it provide the
features that are important to (and indeed, the whole point of) the
BackgroundWorker class. It also still wouldn't implement the threading
the same way that BackgroundWorker does (that class uses the thread pool).

But, inasmuch as adding a Launch() method would parallel the
BackgroundWorker.RunWorkerAsync() and thus would cause your own class's
API to be a little more similar to the BackgroundWorker class, I suppose
you could under some very loose definition of "more or less" be "more or
less the same thing as a BackgroundWorker".

Pete

 

[Harlan Messinger]

[...]
I only recently came across the BackgroundWorker class. Now I'm
thinking that I could add to my AbstractFileProcessor class a method
that creates its own thread and hands the object's own Run method to it,

public virtual void Launch()
{
Thread thread = new Thread(new ThreadStart(this.Run));
thread.Start();
}

(and I could even make the Run method private or protected), and then
the application could call Launch directly without needing to create a
thread first--and if I did that, then my AbstractFileProcessor would
be more or less the same thing as a BackgroundWorker. Is that correct?

That depends entirely on your definition of "more or less". The class
you posted isn't even thread-safe (among other things, I sure hope you
have no client code modifying event subscriptions once you start the
thread executing the Run() method), never mind does it provide the
features that are important to (and indeed, the whole point of) the
BackgroundWorker class. It also still wouldn't implement the threading
the same way that BackgroundWorker does (that class uses the thread pool).

But, inasmuch as adding a Launch() method would parallel the
BackgroundWorker.RunWorkerAsync() and thus would cause your own class's
API to be a little more similar to the BackgroundWorker class, I suppose
you could under some very loose definition of "more or less" be "more or
less the same thing as a BackgroundWorker".

I was thinking along the lines of, did I waste my time, and should I
switch to using a BackgroundWorker.

Agreed, I constructed this class specifically for situations where I
create the processing object, create the object that will monitor it (a
progress form), set the event subscriptions, and then set the process
object running. But of course I'm curious now. What's thread-unsafe
about it? Are you talking specifically about constructs like

if (Ended != null) Ended(source, e);

which, of course, I'm doing without a lock? How would I arrange for the
code that a client would typically use,

TheProcess.SpecialEvent += MySpecialEventHandler;

to coordinate properly with the code the fires the event if it isn't null?

 

[Peter Duniho]

I was thinking along the lines of, did I waste my time, and should I
switch to using a BackgroundWorker.

Without knowing the specific use scenario, I can't say.
BackgroundWorker is mainly useful for dealing with background tasks in
the context of a GUI. Its main contribution is wrapping the
cross-thread invocations on your behalf, so that events are raised in
the original GUI thread.

Of course, in doing so it inherently makes those operations thread-safe.
Thread safety is only an issue where you can access the same data
structure from multiple threads, but with BackgroundWorker the design
implicitly guides you into a situation where only one thread is ever
used to access the data.

Note, of course, that BackgroundWorker doesn't _impose_ thread safety on
your code. You do still have a different thread executing code (the
DoWork event handler) and that thread can still incorrectly access the
same data structures being accessed by the main GUI thread. But as long
as you use the class correctly, using the events to move data back and
forth between the threads, you get thread safety at no additional cost.

Agreed, I constructed this class specifically for situations where I
create the processing object, create the object that will monitor it (a
progress form), set the event subscriptions, and then set the process
object running. But of course I'm curious now. What's thread-unsafe
about it? Are you talking specifically about constructs like

if (Ended != null) Ended(source, e);

which, of course, I'm doing without a lock? How would I arrange for the
code that a client would typically use,

TheProcess.SpecialEvent += MySpecialEventHandler;

to coordinate properly with the code the fires the event if it isn't null?

As long as the client code only modifies the event _before_ the worker
class has started running its thread, it's fine. You just need to make
sure of that.

There are two common idioms for dealing with making events thread-safe:

– Save the event field into a local variable and then invoke the
event via the local variable instead of the event field. For example:

class Class
{
public event EventHandler SomeEvent;

private void RaiseSomeEvent()
{
EventHandler handler = SomeEvent;

if (handler != null)
{
handler(this, EventArgs.Empty);
}
}
}

– Pre-initialize the event field with an empty event handler, to
ensure that it's always non-null. For example:

class Class
{
public event EventHandler SomeEvent = delegate { };

private void RaiseSomeEvent()
{
SomeEvent(this, EventArgs.Empty);
}
}

The latter is somewhat more elegant, but of course has the added
overhead of the anonymous method and delegate instance.

You didn't post the complete code for your class, but if you weren't
specifically designing for thread-safety, you probably haven't declared
the "canceled" field as "volatile", nor is there any synchronization
around the uses of it. So that's not thread-safe either. And of
course, raising events on a thread other than the one the client expects
may or may not be thread-safe, depending on how the client implemented
its event handlers.

There may be other ways in which the implementation isn't thread-safe.
Without a concise-but-complete code example it's not possible to comment
on everything. But those are the most obvious issues I see.

Pete