29

I have a .NET (C#) application that makes extensive use of async/await. I feel like I've got my head around async/await, but I'm trying to use a library (RestSharp) that has an older (or perhaps I should just say different) programming model that uses callbacks for asynchronous operations.

RestSharp's RestClient class has an ExecuteAsync method that takes a callback parameter, and I wanted to be able to put a wrapper around that which would allow me to await the whole operation. The ExecuteAsync method looks something like this:

public RestRequestAsyncHandle ExecuteAsync(IRestRequest request, Action<IRestResponse> callback);

I thought I had it all working nicely. I used TaskCompletionSource to wrap the ExecuteAsync call in something that I could await, as follows:

public static async Task<T> ExecuteRequestAsync<T>(RestRequest request, CancellationToken cancellationToken) where T : new()
{
    var response = await ExecuteTaskAsync(request, cancellationToken);

    cancellationToken.ThrowIfCancellationRequested();

    return Newtonsoft.Json.JsonConvert.DeserializeObject<T>(response.Content);
}

private static async Task<IRestResponse> ExecuteTaskAsync(RestRequest request, CancellationToken cancellationToken)
{
    var taskCompletionSource = new TaskCompletionSource<IRestResponse>();

    var asyncHandle = _restClient.ExecuteAsync(request, r => 
    {
        taskCompletionSource.SetResult(r); 
    });

    cancellationToken.Register(() => asyncHandle.Abort());

    return await taskCompletionSource.Task;
}

This has been working fine for most of my application.

However, I have one part of the application that does hundreds of calls to my ExecuteRequestAsync as part of a single operation, and that operation shows a progress dialog with a cancel button. You'll see that in the code above that I'm passing a CancellationToken to ExecuteRequestAsync; for this long-running operation, the token is associated with a CancellationTokenSource "belonging" to the dialog, whose Cancel method is called if the use clicks the cancel button. So far so good (the cancel button does work).

My problem is that my application's memory usage shoots up during the long-running application, to the extent that it runs out of memory before the operation completes.

I've run a memory profiler on it, and discovered that I have lots of RestResponse objects still in memory, even after garbage collection. (They in turn have huge amounts of data, because I'm sending multi-megabyte files across the wire).

According to the profiler, those RestResponse objects are being kept alive because they're referred to by the TaskCompletionSource (via the Task), which in turn is being kept alive because it's referenced from the CancellationTokenSource, via its list of registered callbacks.

From all this, I gather that registering the cancellation callback for each request means that the whole graph of objects that is associated with all those requests will live on until the entire operation is completed. No wonder it runs out of memory :-)

So I guess my question is not so much "why does it leak", but "how do I stop it". I can't un-register the callback, so what can I do?

svick
  • 236,525
  • 50
  • 385
  • 514
Gary McGill
  • 26,400
  • 25
  • 118
  • 202
  • Excellent question. Though I don't understand where/how the `CancellationTokenSource` references the `Task` or the `TaskCompletionSource`. The callback/closure that is registered on the `cancelationToken` should just capture asyncHandle but nothing more. Can you elaborate on that? I'm having a kind of similar problem but fail to see the connection, so your input might help. – Johannes Rudolph Apr 03 '14 at 15:57
  • Possibly related to [aws-sdk-net issue #361](https://github.com/aws/aws-sdk-net/issues/361). Nice catch! – jweyrich Jun 07 '16 at 17:31

2 Answers2

32

I can't un-register the callback

Actually, you can. The return value of Register() is:

The CancellationTokenRegistration instance that can be used to deregister the callback.

To actually deregister the callback, call Dispose() on the returned value.

In your case, you could do it like this:

private static async Task<IRestResponse> ExecuteTaskAsync(
    RestRequest request, CancellationToken cancellationToken)
{
    var taskCompletionSource = new TaskCompletionSource<IRestResponse>();

    var asyncHandle = _restClient.ExecuteAsync(
        request, r => taskCompletionSource.SetResult(r));

    using (cancellationToken.Register(() => asyncHandle.Abort()))
    {
        return await taskCompletionSource.Task;
    }
}
svick
  • 236,525
  • 50
  • 385
  • 514
  • aha! Thanks, I was looking for an Unregister method - it never occured to me that Register returned a value. – Gary McGill Jan 31 '13 at 15:58
  • PS. Is there any reason you would prefer the try/catch to simply putting `using (var registration = ...) { }`? – Gary McGill Jan 31 '13 at 15:59
  • @GaryMcGill I didn't even think of `using`. Yeah, I think that would be better here. – svick Jan 31 '13 at 16:00
  • 1
    Does CancellationTokenSource.Dispose not deregister all callbacks registered for the cancellation token? – Dave Lawrence Oct 09 '13 at 13:50
  • 1
    @daveL It does, but if I understand the question correctly, the CTS in question is long-lived. Which means that if you don't `Dispose()` the registration, you're going to get a memory leak. – svick Oct 09 '13 at 13:56
  • Yes. I understand that the CTS is long lived... I just got a little bit of a scare because I wasn't disposing these CancellationTokenRegistrations. I was just disposing the CTS. – Dave Lawrence Oct 09 '13 at 14:33
8

You need to keep the CancellationTokenRegistration returned by the Register() call. Disposing that CancellationTokenRegistration un-registers the callback.

MNGwinn
  • 2,394
  • 17
  • 18
  • 1
    @Thanks. I feel bad that I can't accept both your answers, which I seem to have been both made at the same time. I accepted Svick's because it was more comprehensive. Nothing personal... :-) – Gary McGill Jan 31 '13 at 16:01