4
\$\begingroup\$

In my program I need to monitor the state of a static string value that can be changed from different threads in different parts of the system. I came up with the following simple function that, once called, waits 5 seconds to get the value, and if the value is not set, returns null:

// Waits 5 seconds for value to appear and returns this value or null
public async Task<string> GetStringValueOrNull()
{
    var delayTask = Task.Delay(5000);
    var getValueTask = Task.Run(async () => 
    {
        while (true)
        {
            if (!string.IsNullOrEmpty(ExternalValueContainer.Value)) // ExternalValueContainer.Value is 'public static string'
                return ExternalValueContainer.Value;
            await Task.Delay(500);
        }
    });

    var firstCompletedTask = await Task.WhenAny(getValueTask, delayTask);
    if (firstCompletedTask == getValueTask)
    {
        return await getValueTask;
    }
    else
    {
        return null;
    }
}

The function is used like this:

string stringValue = await GetStringValueOrNull();
if (stringValue != null)
    DoSomething(stringValue);
else
    DoSomethingElse();

Can the logic in GetStringValueOrNull be somehow simplified?

\$\endgroup\$
  • \$\begingroup\$ What if the value is set twice within the 500 ms? \$\endgroup\$ – JanDotNet Jun 8 '17 at 14:03
  • \$\begingroup\$ @JanDotNet, in my case it is not relevant. This code is used in unit tests, and it does not make any significant decisions based on the value of the string. It is sufficient for my tests to capture any non-null value during these 5 seconds, \$\endgroup\$ – Andre Borges Jun 8 '17 at 21:03
2
\$\begingroup\$

Some other improvements:

  1. Check ExternalValueContainer.Value synchronously first. If it already exist then you can return early, and there is no reason to await anything. (Note eurotrash's point about thread safety).
  2. If value is never set, getValueTask never returns. Instead of waiting for delayTask separately, you should check the timeout inside getValueTask's body and return once timeout is exceeded.

In general I feel like proper unit tests should not use delays and should not rely on them. Not only because those 5s delays quickly add up if you run this method multiple times, but also because the behavior of unit tests becomes unpredictable. The same test can fail on one machine and pass on another, simply because on one machine GetStringValueOrNull() timed out, while on the other - it did not. IMHO you should take it as signal, that you design is flawed and should be changed. If you own ExternalValueContainer, then you should probably modify its API so consumer can wait for the value (using async methods, events, observables or any other suitable tool). If you do not own ExternalValueContainer then you should wrap it in non-static service and mock this service in your tests.

\$\endgroup\$
1
\$\begingroup\$
        if (!string.IsNullOrEmpty(ExternalValueContainer.Value)) // ExternalValueContainer.Value is 'public static string'
            return ExternalValueContainer.Value;

This is not thread safe. If ExternalValueContainer.Value is not NullOrEmpty, you return it - but the value can change between when you read it before calling IsNullOrEmpty and when you read it to return it. So depending on the implementation of ExternalValueContainer.Value, you could either return a stale value or the latest value which could have become null/empty.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.