Using async void to implement dataprovider

Question

I have an interface like this

public interface IServerDataProvider 
{
    string Val1 { get; }
    string Val2 { get; }

    event EventHandler<EventArgs> Val1Changed;
    event EventHandler<EventArgs> Val2Changed;
}

It gives the user access to two strings retrieved from a server and events that are triggered when these strings change.

Learning about async-await in c#, I can make a fairly simple implementation that periodically checks if these values are changed on a server :

public class ServerDataProviderAsync : IServerDataProvider
{
    public event EventHandler<EventArgs> Val1Changed;
    public event EventHandler<EventArgs> Val2Changed;

    private string _val1Url = "someUrl";
    private string _val2Url = "otherUrl";
    private const int _delayMs = 1000;

    public ServerDataProviderAsync()
    {
        Start();
    }

    private async void Start()
    {
        Val1 = await DownloadString(_val1Url);
        Val2 = await DownloadString(_val2Url);

        Val1UpdateLoop();
        Val2UpdateLoop();
    }

    private async void Val1UpdateLoop()
    {
        await Task.Delay(_delayMs);
        Val1 = await DownloadString(_val2Url);
        Val1UpdateLoop();
    }

    private async void Val2UpdateLoop()
    {
        await Task.Delay(_delayMs);
        Val2 = await DownloadString(_val1Url);
        Val2UpdateLoop();
    }

    private string _val1;
    public string Val1
    {
        get { return _val1; }
        private set
        {
            if (_val1 != value && value != null)
            {
                _val1 = value;
                OnContentChanged(Val1Changed);
            }
        }
    }

    private string _val2;
    public string Val2
    {
        //similar to Val1
    }

    private async Task<string> DownloadString(string url)
    {
        using (var wb = new WebClient())
        {
            try { return await wb.DownloadStringTaskAsync(url); }
            catch { /*log error*/}
        }
        return null;
    }

    private void OnContentChanged(EventHandler<EventArgs> handler)
    {
        if (handler != null)
        {
            handler(this, EventArgs.Empty);
        }
    }
}

And it can be used something like this from MainWindow :

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        var dataProvider = new ServerDataProviderAsync();
        //hook up to events and display strings in GUI
    }
}

Now my question is if this is a good implementaion? Is there a better way?

The first part I'm worried about are the async void methods. I've read they should only be used for event handlers. Are they bad in this case? And if so, why?

The other thing I'm worried about is the recursive way the update loops work. But it seems that since it always awaits tasks that are not already finished, it will not keep adding to the call stack.

Answer 1

You should really use an [iterative] infinite loop to create an infinite loop rather than using infinite recursion.

Using recursion means constantly spending the effort to re-create the exact same state machine from scratch each iteration instead of using the perfectly fine state machine that you already have, and it needlessly obfuscates the code and reduces clarity (to the point that you yourself weren't even sure of the possible negative repercussions; you don't want every single other person who reads the code to have to think through the same problem) for no real gain. Additionally, if you want to be able to propagate exceptions generated in this method to the caller (discussed further below) then using recursion has a number of problems, such as completely messing up the call stack, making actually throwing the exception through all of those levels difficult, and also creating a memory leak in that each "finished" state machine wouldn't be able to be cleaned up.

As for the methods being void, that's not particularly problematic. The reason that one would normally want a Task returned is so that you can tell when the operation finishes. Your operations never finish, they run forever. Getting a task that will never be completed isn't really any more or less useful than not getting a task at all in most circumstances.

The only way it might be relevant is error handling. If your loop generates an error and the method is void it needs to be responsible for handling that error itself, because it is conceptually a top level method. If it returns a Task then it gets the luxury of simply throwing that exception to its caller and leaving that caller responsible for handling that Exception. This would be the only reason to not return void for a method that is supposed to run forever.