Wrap a callback method in an awaitable method

Question

I'm using a .NET library which uses a pre-async/await type of asynchronicity. That is, it provides asynchronous (non-awaitable) method with a callback parameter.

I'm trying to write awaitable extension methods which will wrap some of those methods to make it easier to use with the async/await fashion but I'm not sure if this is the right approach.

Here's an example of one of those methods:

public void OAuthGetRequestTokenAsync(string callbackUrl, Action<FlickrResult<OAuthRequestToken>> callback)
{
    CheckApiKey();
    string url = //...

    Dictionary<string, string> parameters = OAuthGetBasicParameters();
    // [Code to add parameters]

    FlickrResponder.GetDataResponseAsync(this, url, parameters, (r) =>
    {
        var result = new FlickrResult<OAuthRequestToken>();
        if (r.Error != null)
        {
            if (r.Error is System.Net.WebException)
            {
                var ex = new OAuthException(r.Error);
                result.Error = ex;
            }
            else
            {
                result.Error = r.Error;
            }
            callback(result);
            return;
        }
        result.Result = FlickrNet.OAuthRequestToken.ParseResponse(r.Result);
        callback(result);
    });
}

And here's what I came up with to provide an awaitable extension method which wraps the one above:

static class FlickrExtensions
{
    public static Task<OAuthRequestToken> OAuthGetRequestTokenAsync(this Flickr flickr, 
                                                                    string callbackUrl)
    {
        var tcs = new TaskCompletionSource<OAuthRequestToken>();
        flickr.OAuthGetRequestTokenAsync(callbackUrl, r =>
        {
            if (r.HasError)
                tcs.TrySetException(r.Error);
            else
                tcs.TrySetResult(r.Result);
        });

        return tcs.Task;
    }

    // More extension methods to be added.
}

Are there any problems with my code or any downsides to this implementation?

Also, is there a better (more standard?) way to achieve this goal?

Answer 1

Error Handling

You are relying on OAuthGetRequestTokenAsync to catch all exceptions for you.

flickr.OAuthGetRequestTokenAsync(callbackUrl, r =>
{
    if (r.HasError)
        tcs.TrySetException(r.Error);
    else
        tcs.TrySetResult(r.Result);
});

Unhandled errors

However, OAuthGetRequestTokenAsync internally calls FlickrResponder.GetDataResponseAsync, which as you can see in the Reference Source does not catch all exceptions.

Most exceptions are caught and provided to the callback:

// .. snippet from FlickrNet.Internals.FlickrResponder.GetDataResponseAsync()
if (e.Error != null)
{
    result.Error = e.Error;
    callback(result);
    return;
}

But some are thrown to the caller:

// .. snippet from FlickrNet.Internals.FlickrResponder.GetDataResponseAsync()
using (var responseReader = new StreamReader(response.GetResponseStream()))
{
    string responseData = responseReader.ReadToEnd();

    throw new OAuthException(responseData, ex);
}

---

Refactored Code

To wrap this call in an async task, I would also catch these exceptions and handle them in the Task.TrySetException. In addition, since you provide a public API, I suggest to avoid NullReferenceException and check required arguments against null. Perhaps callbackUrl should also be checked early against null. I'm not sure about this, you'd have to verify.

public static Task<OAuthRequestToken> OAuthGetRequestTokenAsync(
    this Flickr flickr, string callbackUrl)
{
    if (flickr == null) throw new ArgumentNullException(nameof(flickr));
    var tcs = new TaskCompletionSource<OAuthRequestToken>();
    try
    {
        flickr.OAuthGetRequestTokenAsync(callbackUrl, r =>
        {
            if (r.HasError)
                tcs.TrySetException(r.Error);
            else
                tcs.TrySetResult(r.Result);
        });
    } 
    catch (Exception uncaughtError)
    {
        tcs.TrySetException(uncaughtError);
    }

    return tcs.Task;
}