I inherited a piece of software. This program is connected to an external hardware, which is a measurement device. Every 100 milliseconds, a value is read from this device and displayed to the user. When the user clicks a button, this continuous measurement is interrupted and no value will be displayed to the user anymore. Some action is done and when this action is finished, the program starts to take the measurement values again. In the code, this realized like this:
private ManualResetEventSlim _isPortReadyEvent = new ManualResetEventSlim(true);
private volatile bool _continueReadingValue;
private int _value;
public int Value
{
get => _value;
set
{
_value = value;
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value)));
}
}
private async Task StartValueReading()
{
_isPortReadyEvent.Reset();
_continueReadingValue = true;
while (_continueReadingValue)
{
Value = await GetValueFromMeasurementDevice();
// assigning the result to property Value displays the result on the GUI
await Task.Delay(100);
}
_isPortReadyEvent.Set();
}
private async Task StopValueReading()
{
_continueReadingValue = false;
await Task.Run(_isPortReadyEvent.Wait);
}
At program start, StartValueReading is executed. When the button is clicked, the following method is executed:
private async Task ExecuteAction()
{
await StopValueReading();
await DoSomething();
StartValueReading();
DoSomeMoreWorkWhileValueIsReadAgain();
}
As described above, in the first line the reading of the value will be stopped. In the second line the action is performed. In the third line the program will start to read the value again. Note that the method is not awaited. This is necessary, because the method is more or less an endless loop. If the call was awaited, the last method would never be called.
The compiler shows a warning that StartValueReading() is not awaited. Although the program works really well in practice, this approach feels wrong to me. It looks like a misuse of async/await. Is this feeling correct? What problems could arise from this approach? Do you think it is necessary to refactor it, maybe to a completely different approach without async/await? (I know exception handling is a problem, but let’s assume GetValueFromMeasurementDevice() will never throw an exception)
4
It looks like a misuse of async/await.
It is.
Do you think it is necessary to refactor it, maybe to a completely different approach without async/await?
Yes.
Every 100 milliseconds, a value is read from this device
I’d just use a Timer. The API of your code and Timer are very similar in two regards:
Timer |
your code | |
|---|---|---|
| a switch to turn the thing on or off | Timer.Enabled |
StartValueReading()/StopValueReading() |
| when the thing is doing something, its expected output is some event being raised | Timer.Elapsed |
PropertyChanged |
Compare this:
private async Task StartValueReading()
{
_isPortReadyEvent.Reset();
_continueReadingValue = true;
while (_continueReadingValue)
{
Value = await GetValueFromMeasurementDevice();
// assigning the result to property Value displays the result on the GUI
await Task.Delay(100);
}
_isPortReadyEvent.Set();
}
to something like that:
private void StartValueReading()
{
_measurementTimer.Enabled = true;
}
I inherited a piece software.
And it smells. Bad.
private volatile bool _continueReadingValue;
Do you (think you) know why this field is volatile? I’d follow Eric’s advice “I leave the usage of “volatile” to real experts.” 😉 in a nutshell (SO answer summary) or in a watermelon (source blog post)
But you don’t have to go to such low level.
What happens if Value is assigned the same value twice?
How many property changed events would you expect? How many do you get?
public int Value
{
get => _value;
set
{
_value = value;
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value)));
}
}
These indicate (to me) that previous authors of the code might not have had the time, experience, knowledge and/or mindset to write code that smells less. async/await was a somewhat advanced language feature when introduced, too advanced you might say for some developers.
Its usage is probably as sensible as the rest of the legacy code base you are dealing with.
0
I think you are awaiting the result somewhere: in StopValueReading. Except rather than doing so through the Task you have to define your own semaphore to keep track of it. You could eliminate _isPortReadyEvent by just awaiting the result of StartValueReading() in StopValueReading(), like this:
Task _valueReadingTask;
private async Task ExecuteAction()
{
await StopValueReading();
await DoSomething();
_valueReadingTask = StartValueReading();
DoSomeMoreWorkWhileValueIsReadAgain();
}
private async Task StopValueReading()
{
_continueReadingValue = false;
await _valueReadingTask;
}
You could also provide a cancellation token, which would potentially enable you to stop the task immediately without having to wait for the 100ms Wait to expire. However this causes the awaiter to throw an OperationCancelledException, which not everybody likes.
3
Generally the code is ok. There’s a slight risk that if you refactor StartValueReading to be say synchronous, then you end up blocking yourself. In particular ExecuteAction needs to be aware of the details of StartValueReading. This can be avoided by wrapping the call with say Task.Run().
But I don’t think you will get rid of the warning either way. However warnings are just that: warnings. The compiler tries to tell you: are you sure this is not a mistake? Because most of the time you should await a task. But not always. Suppress it, move on.
There is a proper way of handling this. Create a background worker class, that manages the background task and implements say IAsyncDiposable that does the cleanup (including the final await). The cleanup can be called on application shutdown for example. That mitigates another risk of potential dangling background worker. But that’s likely to be lots of work, not necessarily of any benefit. I would stick with YAGNI in this case.
6