9
\$\begingroup\$

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.

/// <summary> /// Waits for multiple concurrent operations to finish before firing a callback /// </summary> public class TtConcurrentSynchronizer { #region Properties private readonly List<Callback> callbacks = new List<Callback>(); /// <summary> /// The callbacks that must all be executed before the finished function is fired. /// </summary> public IEnumerable<Action> Callbacks { get { return callbacks.Select(x => CreateCallbackAction(x)); } } #endregion Properties #region Private Fields private Action finishedFunction; #endregion Private Fields #region Constructors /// <summary> /// Creates a new instance of the concurrent synchronizer /// </summary> /// <param name="finishedFunction"> /// Method that is executed when all assigned asynchronous methods have completed. /// </param> public TtConcurrentSynchronizer(Action finishedFunction) { this.finishedFunction = finishedFunction; } #endregion Constructors #region Public Methods /// <summary> /// Create a callback function to send to the asynchronous method /// </summary> /// <returns> A callback action that invokes an internal checking function </returns> public Action CreateCallBack() { var callback = new Callback(); var callbackAction = CreateCallbackAction(callback); callbacks.Add(callback); return callbackAction; } /// <summary> /// Create a callback function to send to the asynchronous method, combined with a provided function /// </summary> /// <param name="additionalFunction"> /// An additional function to call on the method's completion /// </param> /// <returns> /// A callback action that invokes an internal checking function as well as an additional function /// </returns> public Action CreateCallBack(Action additionalFunction) { var callback = new Callback(additionalFunction); var callbackAction = CreateCallbackAction(callback); callbacks.Add(callback); return callbackAction; } /// <summary> /// Create an action that removes a callback from the callbacks array and fires its callback /// method if it hasn't already been removed. /// </summary> /// <param name="callback"> The callback to create an action for. </param> private Action CreateCallbackAction(Callback callback) { return () => { if (callbacks.Contains(callback)) { if (callback.CallbackMethod != null) { callback.CallbackMethod(); } callbacks.Remove(callback); if (!callbacks.Any()) { finishedFunction(); } } }; } #endregion Public Methods #region Private Classes /// <summary> /// Represents a concurrency callback function. /// </summary> private class Callback { #region Private Fields private readonly Guid id = Guid.NewGuid(); #endregion Private Fields #region Public Properties private Action callbackMethod = null; /// <summary> /// Gets the method executed when the callback is called. /// </summary> public Action CallbackMethod { get { return callbackMethod; } } /// <summary> /// Gets a unique identifier for this callback. /// </summary> public Guid Id { get { return id; } } #endregion Public Properties #region Public Constructors /// <summary> /// Initializes a new Callback instance. Use this constructor when you want additional /// functionality on callback. /// </summary> public Callback(Action callbackMethod) { this.callbackMethod = callbackMethod; } /// <summary> /// Initializes a new Callback instance. Use this constructor when you don't want any /// additional functionality on callback. Equivalent to new Callback(null); /// </summary> public Callback() { } #endregion Public Constructors } #endregion Private Classes }

Once again, typical usage is as follows:

Action finishedFunction = LongProcessFinished; var concurrentSynchronizer = new ConcurrentSynchronizer(finishedFunction); BackgroundWorker longProcess1 = new BackgroundWorker(); var longProcess1Callback = concurrentSynchronizer.CreateCallback(); longProcess1.DoWork += (o,e) => { DoLongProcess(); }; longProcess1.RunWorkerCompleted += (o, e) => longProcess1Callback(); //With an additional callback Action longProcess2AdditionalCallback = AdditionalCallback; var longProcess2 = new BackgroundWorker(); Action longProcess2Callback = concurrentSynchronizer.CreateCallback(longProcess2AdditionalCallback); longProcess2.DoWork += (o,e) => { DoLongProcess2(); }; longProcess2.RunWorkerCompleted += (o, e) => longProcess2Callback(); longProcess1.RunWorkerAsync(); longProcess2.RunWorkerAsync();

I was also asked why I did not use Tasks or WaitHandle.WaitAll(). The reasons are:

  • I aim to target .NET 3.5, so Tasks are out of the equation.
  • I did not like the intrusive syntax of WaitHandles, with my system I can use existing asynchronous operations without modifying them in any way, including WCF calls and the like.
\$\endgroup\$

2 Answers 2

Reset to default
6
+50
\$\begingroup\$

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:

  1. Load the value - all the threads will see the same value, e.g. 3
  2. Do the math = decrement - all the threads will calculate 2
  3. 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

  1. Semaphore with one waiting/sleeping Thread to finally dispatch the finish action.
  2. 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(callback)) return; } // call the actions outsid of the lock-block if (callback.CallbackMethod != null) callback.CallbackMethod(); lock(callbacks) // another lock for remove and check { callbacks.Remove(callback); if (callbacks.Any()) return; } // outside of lock finishedFunction(); }; }

The above code may not be optimal, but makes sure that all the actions are executed first and finishedFunction last (different solution with single lock could reorder them).

\$\endgroup\$
9
  • \$\begingroup\$ ConcurrentSynchronizer doesn't spawn any workers. The workers are spawned by the programmer. Concurrentsynchronizer merely creates callback functions that are to be applied on Finished events, or sent in as parameters. Use of a BackgroundWorker is merely an example, it also works on WCF domain operations. EDIT: Also it is absolutely not designed for UI / main thread usage only. I really don't know where you get this idea from. It can be used anywhere from any thread. It is simply a helper for waiting for multiple asynch processes to finish before calling some other code. \$\endgroup\$ Commented Oct 8, 2014 at 14:31
  • \$\begingroup\$ I know the synchronizer is not spawning any workers, thats why I was talking about separation. The thing is, that if it is not designed to be used by main thread only (and workers and similar), then it may be designed for threads, but in such case it is broken (lack of synchronization). \$\endgroup\$ Commented Oct 8, 2014 at 14:34
  • \$\begingroup\$ Ah I see, I had misunderstood. What causes a lack of synchronization here? It's working for me on multiple asynchronous processes (WCF domain invokes). Perhaps I should rename my class? It is only to synchronize thread endpoints to start a new function, perhaps ConcurrentWaiter is a better name? EDIT: Ah, I think there might be some confusion here. Do you think this is for synchronizing actively running threads? Or perhaps for managing access to state? This is intended for calling function d when asynchronous functions a, b and c have all completed. \$\endgroup\$ Commented Oct 8, 2014 at 15:08
  • \$\begingroup\$ The problem here is: which thread is calling which action/callback? I was assuming that your code is working, that assumption led me to ask why and how can it work without single lock. After examining usage example, I got an answer: because the BackrowndWorker (as a nice Component) is doing all the synchronization for you! Imagine Worker that would execute the Completed action in the same thread as DoWork. Random, unexpected crash after thousand successful runs. \$\endgroup\$ Commented Oct 8, 2014 at 15:23
  • \$\begingroup\$ Ah, I understand now. The parent thread (which created the async ops) is calling the callbacks by attaching to various finished events on the async ops (e.g BackgroundWorker.RunWorkerCompleted or InvokeOperation.Completed, etc.) which means I can run, say, 10 background workers and then whatever I set as FinishedFunction is only called when all 10 are complete. But this FinishedFunction is called on the parent thread. \$\endgroup\$ Commented Oct 8, 2014 at 15:34
4
\$\begingroup\$

I will try to give you another review, once we made clear how it was designed and where is the biggest problem (in short: thread synchronization)

What I like

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.

Given the sample of the code:

/// <summary> /// Waits for multiple concurrent operations to finish before firing a callback /// </summary> public class TtConcurrentSynchronizer { #region Properties private readonly List<Callback> callbacks = new List<Callback>(); /// <summary> /// The callbacks that must all be executed before the finished function is fired. /// </summary> public IEnumerable<Action> Callbacks { get { return callbacks.Select(x => CreateCallbackAction(x)); } } #endregion Properties #region Private Fields private Action finishedFunction; #endregion Private Fields

I would rather use classic Doxygen (or Javadoc) style, but we are in Microsoft's world - have to follow the rules. Those #region..#endregion are nice because you can collapse them. I usually add separating comments and indent them on one tab less:

/// <summary> /// Waits for multiple concurrent operations to finish before firing a callback /// </summary> public class TtConcurrentSynchronizer { //====================================== depends on your prefered width, e.g. 80 or 120 #region Properties private readonly List<Callback> callbacks = new List<Callback>(); /// <summary> /// The callbacks that must all be executed before the finished function is fired. /// </summary> public IEnumerable<Action> Callbacks { get { return callbacks.Select(x => CreateCallbackAction(x)); } } #endregion Properties //====================================== depends on your prefered width, e.g. 80 or 120 #region Private Fields private Action finishedFunction; #endregion Private Fields

Because after collapsing you will see something like this:

+ #region Properties ... //======================================= + #region Private Fields ...

And those separating comments help with scrolling while the regions are expanded.

One day you will have to manage your code. Or worse, one day somebody else will have to manage your code. I am often not documenting my code enough when I am in a hurry and need to get the job done, but once there is the time for it, I clean and comment the code (especially when my stomach is full after a lunch and my brain refuses to work :D).

What I don't like

Pardon me, but the complexity, unclear interface and all those closures:

private Action CreateCallbackAction(Callback callback) { return () => { if (callbacks.Contains(callback))

Do you really need to check if callbacks.Contains(callback)? Who can remove it before it gets executed?

What I would like to see is one simple class (this is .NET not Java, I hate connecting small pieces in javax.swing - pardon me again if you like that). Your helper is fine in doing the job, but for me, it is internal, your helper and I want the final, easy to use class:

class WorkScheduler { public WorkScheduler() {...} // create it to fill the jobs later public WorkScheduler(Action finished, params Action[] jobs) // all-in-one public WorkScheduler(Action finished, params WorkAndFinishedPair[] jobs) // more complex public void Start() // to be used with default constructor public event Action FinalAction; // add the action if default-constructing public event Action Jobs; // add jobs by += public void AddJob(Action job) // the same as above with += public void AddJob(Action job, Action finished) // the more complex version

All I have seen in your question was using the synchronizer together with BackgroundWorkers. The above should be the interface to connect both together. If you want to add something else, create another class using your helper.

public void Start() { jobCounter = jobs.Count; // atomic/interlocked downcounter foreach(var pair in jobs) { var worker = new BackgroundWorker(); worker.DoWork += pair.JobToDo; // with possible conversion worker.RunWorkerCompleted += pair.FinishAction != null ? ... : ...; worker.RunWorkerAsync(); // we can start it now, // because we have initialized the counter first } }

You could as well use Threads instead of BackgroundWorkers but remember what you have learned about synchronization and ISynchronizeInvoke.BeginInvoke (you cannot acces Controls from different threads).

\$\endgroup\$
4
  • \$\begingroup\$ The check to make sure the callback is in callbacks is because I have to prevent the same callback being fired twice, which would attempt a duplicate removal and fire an exception. I'm currently unsure if I want this to be exceptional behaviour or not. I'm eager to hear your thoughts on that. The reason I kept this class small is partly for separation of concerns. Technically there's nothing stopping me using this to manage state based on synchronous calls (for example, SetBusy() and SetFree()), although then the class needs renaming. \$\endgroup\$ Commented Oct 11, 2014 at 14:57
  • 1
    \$\begingroup\$ My point is: can that really happen? Because I don't think so. It would probably mean some serious design flaw, the callback must always be in callbacks at that point or there is some serious error. Maybe I am wrong in this, but that is what I would expect. \$\endgroup\$ Commented Oct 11, 2014 at 16:23
  • \$\begingroup\$ Good point on the serious design error. Perhaps that should be the right place to fire an exception. It can definitely happen, but you've convinced me that it shouldn't \$\endgroup\$ Commented Oct 11, 2014 at 17:24
  • 1
    \$\begingroup\$ Just an idea: What about enclosing your class as protected nested within some abstract/empty WorkerBlockBase and then derive specialized AsyncWorkerBlock (multiple threads), SyncWorkerBlock (single chain), BackgroundWorkerBlock (the example) ...? ...just my thoughs flying around :D \$\endgroup\$ Commented Oct 11, 2014 at 18:33

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.