9

I have a button which has an async handler which calls awaits on an async method. Here's how it looks like: 

private async void Button1_OnClick(object sender, RoutedEventArgs e)
{
    await IpChangedReactor.UpdateIps();
}

Here's how IpChangedReactor.UpdateIps() looks:

public async Task UpdateIps()
{
    await UpdateCurrentIp();
    await UpdateUserIps();
}

It's async all the way down.
Now I have a DispatcherTimer which repeatedly calls await IpChangedReactor.UpdateIps in its tick event.

Let's say I clicked the button. Now the event handler awaits on UpdateIps and returns to caller, this means that WPF will continue doing other things. In the meantime, if the timer fired, it would again call UpdateIps and now both methods will run simultaneously. So the way I see it is that it's similar to using 2 threads. Can race conditions happen? (A part of me says no, because it's all running in the same thread. But it's confusing)

I know that async methods doesn't necessarily run on separate threads. However, on this case, it's pretty confusing.

If I used synchronous methods here, it would have worked as expected. The timer tick event will run only after the first call completed.

Can someone enlighten me?

i3arnon
  • 113,022
  • 33
  • 324
  • 344
wingerse
  • 3,670
  • 1
  • 29
  • 61
  • 6
    The term you are struggling to find is "[Reentrancy](https://en.wikipedia.org/wiki/Reentrancy_(computing))": "*In computing, a computer program or subroutine is called reentrant if it can be interrupted in the middle of its execution and then safely called again ("re-entered") before its previous invocations complete execution.*" – Scott Chamberlain Feb 15 '16 at 21:56

2 Answers2

6

Since both calls run on the UI thread the code is "thread safe" in the traditional sense of - there wouldn't be any exceptions or corrupted data.

However, can there be logical race conditions? Sure. You could easily have this flow (or any other):

UpdateCurrentIp() - button
UpdateCurrentIp() - Timer
UpdateUserIps() - Timer
UpdateUserIps() - button

By the method names it seems not to really be an issue but that depends on the actual implementation of these methods.

Generally you can avoid these problems by synchronizing calls using a SemaphoreSlim, or an AsyncLock (How to protect resources that may be used in a multi-threaded or async environment?):

using (await _asyncLock.LockAsync())
{
    await IpChangedReactor.UpdateIps();
}

In this case though, it seems that simply avoiding starting a new update when one is currently running is good enough:

if (_isUpdating) return;

_isUpdating = true;
try
{
    await IpChangedReactor.UpdateIps();
}
finally
{
    _isUpdating = false;
}
Community
  • 1
  • 1
i3arnon
  • 113,022
  • 33
  • 324
  • 344
  • How about something like `if(_isUpdating) return; _isUpdating = true` in the method beginning and `_isUpdating = false` in the method end? – wingerse Feb 15 '16 at 22:02
  • Using a lock wouldn't really be appropriate if you have work in a timer that takes longer than the timer's tick to happen. You'd want to just not fire new invocations until the previous have finished, else you'll just get a continually increasing backlog. – Servy Feb 15 '16 at 22:03
  • @Servy That's true, and there are ways to deal with it. But really you shouldn't use a timer to begin with. You should use a periodic task that executes and then awaits with `Task.Delay`. – i3arnon Feb 15 '16 at 22:04
  • @i3arnon There's nothing wrong with using a timer. Using `Delay` would have different semantics. Which semantics would actually be better would depend on what the actual requirements are. Timers do still have their place, and it's entirely possible that it's appropriate here. – Servy Feb 15 '16 at 22:06
  • @EmpereurAiman If you don't care how many times the operation will run, only that it ran recently (i.e. refresh) then this should work as well. – i3arnon Feb 15 '16 at 22:06
  • @Servy using a `Timer` where the work is longer than the interval will have the semantics of reentrancy as the event can't be awaited by the caller. Whether you `return;`, lock or use `Task.Delay` you're changing the semantics in some kind, and that's ok as you should change it. – i3arnon Feb 15 '16 at 22:18
  • @i3arnon There are a number of possible behaviors that the code could have. It could be correct to run the code at a set interval, but not fire it if the previous iteration is still running. If that's the semantics that the OP wants, your code doesn't provide it. You're suggesting running the code with a fixed interval between the end of the previous execution and the start of the next. If that's what he wants, fine, if it's not, then that doesn't accomplish it. Neither is right or wrong in general, but they're different behaviors. You choose the one that behaves as you want. – Servy Feb 15 '16 at 23:56
  • @Servy, 1. My code does provide exactly what the OP wants. 2. It's definitely a "better practice" when you're implementing a refresh mechanism to wait between iterations instead of blindly using a timer's interval. 3. A comment you disagree with is not a good reason to downvote an answer (assuming it's yours). – i3arnon Feb 16 '16 at 06:45
  • Both mechanisms are reasonable in different situations. In some situations one is better, and in some the other is better. Neither is universally better than the other. Changing the requested semantics from the requirements is *not* providing exactly what the OP requested. – Servy Feb 16 '16 at 15:55
  • @Servy The semantics are **wrong** if the action can be longer the timeout. You **must** change the semantics in *some* way. One of my suggestions are **exactly** what OP wants. – i3arnon Feb 16 '16 at 16:05
  • The semantics of *just locking* is *probably* wrong. **That's what I have been telling you from the start**. *You* are the one suggesting that in your answer. I'm not advocating just locking. Only firing a handler of a timer when a previous handler isn't running would likely be fine. Proposing waiting a fixed time between the end of the task and the start of the next would work, but it's a deviation from the OP's stated requirements, so he should only be making that change if he wants, or is okay with, the altered semantics. – Servy Feb 16 '16 at 16:35
  • @Servy the semantics of locking are **perfectly fine** for the general case of async reentrancy. It answers the OP's question without assuming the content of the methods which OP didn't post. When OP commented about his **specific** case I amended the answer for his **specific** case. What you've been doing from the start is badgering about a case which isn't OP's case, which is problematic to begin with, is solvable in many ways, all of them change the semantics and is generally irrelevant. – i3arnon Feb 16 '16 at 16:58
4

I can think of a number of ways to handle this issue

1 Do not handle it

Like i3arnon says it might not be a problem to have multiple calls to the methods running at the same time. It all depends on the implementation of the update methods. Just like you write, it's very much the same problem that you face in real, multi-threaded concurrency. If having multiple async operations running at once is not a problem for these methods, you can ignore the reentrancy issues.

2 Block the timer, and wait for running tasks to finish

You can disable the timer, och block the calls to the event handler when you know you have a async task running. You can use a simple state field, or any kind of locking/signaling primitive for this. This makes sure you only have a single operation running at a given time.

3 Cancel any ongoing async operations

If you want to cancel any async operations already running, you can use a cancellationtoken to stop them, and then start a new operation. This is described in this link How to cancel a Task in await?

This would make sense if the operation takes a long time to finish, and you want to avoid spending time to complete an operation that is already "obsolete".

4 Queue the requests

If it's important to actually run all the updates, and you need synchronization you can queue the tasks, and work them off one by one. Consider adding some sort of backpressure-handling if you go down this route...

Community
  • 1
  • 1
Mikael Nitell
  • 1,069
  • 6
  • 16