Concurrent Task Waiter 2

This is an iteration of my previous question: Concurrent Task Waiter

Summary from before:


I have some code designed to simplify managing multiple asynchronous
operations. The code creates callback actions that, when executed by
the asynchronous operation, track which asynchronous methods have
completed. When all have completed, the class executes a
specially-assigned method.


The purpose of this is to wait for multiple resources to load, without
chaining them, in projects that do not have access to async / await.


One note on style: The egregious use of regions and over-commenting is
company style, I'm afraid, and I can't change it. Otherwise I'm
looking for advice on style, efficiency, security, and particularly
type safety.

I've followed robertfriberg's advice and altered the order of execution of the Finished function, as well as introduced a guarantee that when a callback is executed, it is removed from the list and cannot be executed twice.

```
///
/// Waits for multiple concurrent operations to finish before firing a callback
///
public class TtConcurrentSynchronizer
{
#region Properties

private readonly List callbacks = new List();

///
/// The callbacks that must all be executed before the finished function is fired.
///
public IEnumerable Callbacks
{
get { return callbacks.Select(x => CreateCallbackAction(x)); }
}

#endregion Properties

#region Private Fields

private Action finishedFunction;

#endregion Private Fields

#region Constructors

///
/// Creates a new instance of the concurrent synchronizer
///
///
/// Method that is executed when all assigned asynchronous methods have completed.
///
public TtConcurrentSynchronizer(Action finishedFunction)
{
this.finishedFunction = finishedFunction;
}

#endregion Constructors

#region Public Methods

///
/// Create a callback function to send to th

How it works

At first it was very confusing how it really works, but I think I got the idea: You are taking advantage of how BackgroundWorker.RunWorkerCompleted is dispatched - through main message loop (the same way as Control implements ISynchronizeInvoke interface). This allows you to have absolutely no lock in your code but still make it thread-safe (as your synchronizer is not designed to be accessed from any other but main/UI thread).

EDIT: The importat thing here to understand is that BackgroundWorker is executing all the events except DoWork in main/UI thread - that makes it working. See comments and description bellow for synchronization needed if used with threads.

Synchronizer and BackgroundWorkers speration

I don't know why you designed it that way and if you plan to wrap it all in another helper that will be spawning the workers while connecting the callbacks as well, but I think it would be a good idea, because the synchronizer is quite unclear (how it works, how is it to be used).

EDIT: I was at first assuming that the code is fine and working and therefore that it is designed that way - to be used with BackgroundWorker that is doing all the hard synchronization.

The Main/UI thread only design

I think that the main problem with your class is, that it is not clear that it is designed for main/UI thread only. To make it clear (and for better usage), you should derive it from Component the same way as BackgroundWorker is (and probably join it all together as already suggested).

EDIT: When used with BackgroundWorker, all those actions/callbacks are executed on main/UI thread (no matter which thread started it). That is the nice thing about BackgroundWorker and why it is a Component - it is designed to run some job in the background (some thread) but report progress and completition in main thread that we can happily acces our controls without problems.

Why it has to be used with BackgroundWorker?

Imagine Worker that would simply spawn some thread, execute DoWork and finally Completed on the same thread (not on main thread). That could lead to problems inside Remove called on the list of callbacks. Imagine that it has to decrement the Count and is doing it with some math:

• Load the value - all the threads will see the same value, e.g. 3

• Do the math = decrement - all the threads will calculate 2

• Store the value back - 2, but the result should be 0 if three threads were removing their callbacks!

(Synchronization will be described later)

Alternatives

The main question is wheter you really need the BackgroundWorker especially for its nice ReportProgress -> ProgressChanged (which is again dispatched through main message loop).

The alternative I would personally choose is to spawn Thread for each job (BackgroundWorker is doing that for you, maybe using ThreadPool) and use

• Semaphore with one waiting/sleeping Thread to finally dispatch the finish action.

• Or atomic Interlocked downcounter that last job will know to dispatch the finish action.

You will need to use that ISynchronizeInvoke interface (e.g. BeginInvoke method) to synchronize final action with main UI thread (simple if(InvokeRequiered) BeginInvoke(...); else ... would do).

As the comments suggest that it is not designed for main thread only, then look at this Action returned:

private Action CreateCallbackAction(Callback callback)
{
    return () =>
    {
        if (callbacks.Contains(callback))
        {
            if (callback.CallbackMethod != null)
            {
                callback.CallbackMethod();
            }
            callbacks.Remove(callback);
            if (!callbacks.Any())
            {
                finishedFunction();
            }
        }
    };
}

What if two threads execute it simultaneously? What you have to add is lock(callbacks) somewhere, but can the actions be executed while holding the lock? Maybe not, maybe you should examine callbacks under lock, set some variables and execute the actions out of synchronization block.

How to synchronize?

We have to protect anything that we may access from different threads. The common practice is to have some readonly object that we can use for lock. The list of callbacks is exactly what we need. So, any access can be protected this way:

public Action CreateCallBack(Action additionalFunction)
{
    lock(callbacks) {
        var callback = new Callback(additionalFunction);
        var callbackAction = CreateCallbackAction(callback);
        callbacks.Add(callback);
        return callbackAction;
    }
}

The CreateCallbackAction already pointed out is a bit more complex, because we are executing unknown actions - we should not do that in synchronization block (under the lock):

```
private Action CreateCallbackAction(Callback callback)
{
return () =>
{
lock(callbacks) // access callbacks under the lock
{
if (!callbacks.Contains(call

StackExchange Code Review Q#64505, answer score: 5