if statement is meant to check whether a thread is already executing the loop by checking whether
cancellationTokenSource is null; if it's not null, then signal that a sync is required by setting
syncRequired to true and returning. The thread executing the loop will then run another loop due to the
while (dataWasSynchronised || syncRequired ) condition.
cancellationTokenSource is null, then the sync loop isn't being executed, so this thread goes on to do just that.
private async void StartSynchronisation()
// the cancellation token operates as the flag to indicate if the operation is running
if (this.cancellationTokenSource != null)
syncRequired = true;
this.cancellationTokenSource = new System.Threading.CancellationTokenSource();
So the first problem is that that check (the first
if statement) isn't synchronised. One thread can see
cancellationTokenSource is null and move on to execute the sync loop, first setting
cancellationTokenSource to a new object. Before it assigns the new object - say, after setting
syncRequired to true - there's a context switch, and another thread executes the same code. It too sees that
cancellationTokenSource is null, sets
syncRequired to true, assigns a new object to
cancellationTokenSource, and starts running the loop. Then there's another context switch and our original thread is back again, assigns a new object to
cancellationTokenSource, and starts executing the loop as well. Voila, you've now violated your first requirement of having zero concurrency.
The fix is simple: the check, as well as any objects being altered to affect the outcome of the check, must be synchronised.
A basic way to achieve this:
private readonly object syncLock = new object();
private volatile bool syncRequired = false;
private volatile bool syncRunning = false;
private void MySyncFunc()
syncRequired = true;
syncRunning = true;
syncRequired = false;
// your sync operations here
syncRunning = false;
I'll quickly point out this isn't the most efficient or pretty, but it's a simple (and hopefully accurate) way to show and do it.
First of all notice you can't even check whether your sync loop is running without getting the lock. So only one thread at a time can check. If a thread determines it isn't running, it sets the value to true (to signal that it's now running) and exits the lock. The second thread that arrived on the scene in the earlier example, if it was context-switched to during that time, would not have been able to proceed because it couldn't get the lock. Once the first thread exits the lock, the second thread gets the lock. Now it sees that
syncRunning is true, so it sets
syncRequired to true and returns. No sync problems there.
Now let's look at the loop. Nevermind that it's an infinite loop, I just wanted to get the checks out of the loop condition. So first thing it does is set
syncRequired to false. This doesn't need to be synchronised because even if another thread had set it to true right before, you're gonna go and do the actual synchronisation right now anyway.
So after your own sync stuff completes, it's time to check whether you can exit the loop or need to repeat it. (I've left out checking the result of your sync operation as it's not relevant to this.) Again you got to get the lock. Why? Because the thread could check
syncRequired and see it's false, and move on to set
syncRunning = false. But before it's set, context switch, another thread checks and sees that
syncRunning == true, so sets
syncRequired to true and returns. Meanwhile the first thread continues to set
syncRunning to false and returns as well. Now you have
syncRequired == true but no thread running the loop. Whoops, you missed an event! Take a look at your original code and you'll see this problem is there, if
syncRequired is set to true after the loop exits and before
cancellationTokenSource is set to null in the
So what the code above does instead is it gets a lock on the same object again and checks whether a sync is required. If it is, fine, the lock is exited and the loop starts over. But if it's not, then it sets
syncRunning to false before it exits the lock. Since any other thread that wants to check if
syncRunning == true has to wait until the lock in the loop is exited, they will see the correct value when they finally do get the lock, because if the first thread will exit the loop, it will set
syncRunning to false while it has the lock and before it exits the lock. Whether the next thread gets the lock and checks, it will see that
syncRunning == false, and go on to run the loop itself. No more lost events!
Couple other issues:
If you're going to use a member variable in a loop, but not modify its value in the loop (or only ever set it to a single value on each iteration such that there is no apparent point in doing so, even though you know that some other thread can modify the value), then mark it
volatile. This will result in the value being read from memory each time, so you pick up changes made from other threads. Otherwise the compiler could optimise the read out of the loop; after all, it's never changing during the lifetime of the loop.
And your code isn't cancellable. Sure you create a
CancellationTokenSource, but you never actually use it - hence why I simply substituted it with a
bool in my example. What you need to be doing is checking the token each iteration (or possibly pass it to the function being called if that's the part that should allow cancellation of its task, though you'd then have to alter the function to accept and know how to use the token) and seeing whether a cancellation has been requested; then if so, throw a
OperationCanceledException (or however you want to abort what's currently running).