Wrapping a library that uses the Event-Based Asyncronous Pattern, for use with Async/Await

Question

I'm using the async/await pattern throughout my code. However, there's one API which used the event-based asyncronous pattern. I've read on MSDN, and several StackOverflow answers, that the way to do this is to use a TaskCompletionSource.

My code:

public static Task<string> Process(Stream data)
{
    var client = new ServiceClient();
    var tcs = new TaskCompletionSource<string>();

    client.OnResult += (sender, e) =>
    {
        tcs.SetResult(e.Result);
    };

    client.OnError += (sender, e) =>
    {
        tcs.SetException(new Exception(e.ErrorMessage));
    };

    client.Send(data);

    return tcs.Task;
}

And called as:

string result = await Process(data);

Or, for testing:

string result = Process(data).Result;

The method always returns very quickly, but neither of the events get triggered.

If I add tcs.Task.Await(); just before the return statement, it works, but this doesn't provide the asyncronous behavior I want.

I've compared with the various samples I've seen around the internet, but don't see any difference.

Your events are falling out of scope when when the ServiceClient is disposed/destructed. This is happening because the thread (async task) doesn't wait for completion. Try adding AutoResetEvent to your task so that it will wait for completion of one of the event handlers. — The Sharp Ninja, Dec 05 '15 at 14:30

Yuval Itzchakov · Accepted Answer · 2015-12-05T15:42:02.463

8

The problem lays in the fact that after your Process method terminates, your ServiceClient local variable is eligible for garbage collection and may be collected prior to the event firing, hence a race condition is in place.

To avoid this, I'd define ProcessAsync as an extension method on the type:

public static class ServiceClientExtensions
{
    public static Task<string> ProcessAsync(this ServiceClient client, Stream data)
    {
        var tcs = new TaskCompletionSource<string>();

        EventHandler resultHandler = null;
        resultHandler = (sender, e) => 
        {
            client.OnResult -= resultHandler;
            tcs.SetResult(e.Result);
        }

        EventHandler errorHandler = null;
        errorHandler = (sender, e) =>
        {
            client.OnError -= errorHandler;
            tcs.SetException(new Exception(e.ErrorMessage));
        };

        client.OnResult += resultHandler;
        client.OnError += errorHandler;

        client.Send(data);
        return tcs.Task;
    }
}

And consume it like this:

public async Task ProcessAsync()
{
    var client = new ServiceClient();
    string result = await client.ProcessAsync(stream);
}

Edit: @usr points out that usually, IO operations should be the ones keeping the reference to whom invoked them alive, which isn't the case as we're seeing here. I agree with him that this behavior is a bit peculiar, and should probably signal of some sort of design/implementation problem of the ServiceClient object. I'd advise, if possible, to look up the implementation and see if there is anything that might be causing the reference to not stay rooted.

edited Dec 05 '15 at 15:42

answered Dec 05 '15 at 14:35

Yuval Itzchakov

146,575
32
257
321

Are you sure the IO operation doesn't keep the client rooted? They usually all do. Also, that new extension version should not keep anything rooted either. – usr Dec 05 '15 at 14:53
I don't see how the IO operation roots the object, although I actually have no idea how his object works. Especially due to the fact that once he synchronously blocks on the operation, it works. My version will root `ServiceClient` on the stack as he `await`'s on the operation. – Yuval Itzchakov Dec 05 '15 at 14:55
The stack var is unused so it's not a root. Also, it gets captured into a heap closure objects which is unrooted, too. The stack is gone after that point. No roots. – usr Dec 05 '15 at 14:56
@usr `client` (in the second code snippet) will get lifted onto the state-machine, which is itself also a`struct` object, not a heap closure object. In the case of his `Process` method, that won't happen as it encapsulates the `client` object. – Yuval Itzchakov Dec 05 '15 at 15:00
When the await hits the struct gets boxed, the stackframe is destroyed. Nobody's going to root that heap object now. Also, even if the stack frame lived `client` is considered dead by the GC as soon as the call to ProcessAsync starts. It's possible to collect objects while an instance method is running. – usr Dec 05 '15 at 15:01
@usr Actually, re-reading [this post](http://stackoverflow.com/questions/26957311/why-does-gc-collects-my-object-when-i-have-a-reference-to-it), the `Task` exposed by `tcs.Task` should be keeping the state machine alive, shouldn't it? But since in the OP's code, by the time we reach `tcs.Task`, `client` might already be gone. By making it an extension method, the `client` object also be captured and alive throughout the life cycle of the state-machine. – Yuval Itzchakov Dec 05 '15 at 15:08
I still don't believe it. It was marked as the answer but I think neither the rooting works out (reliably) nor is the ServiceClient eligible for collection during IO. IO usually keeps things alive, for example `new FileStream(...).WriteAsync(...);` will never be aborted mid way or something. All continuations will always run. – usr Dec 05 '15 at 15:21
@usr Regarding the IO, I as well don't know, but regarding rooting I don't see why this shouldn't work. This is almost how every EAP method gets translated into async-await. I'll try reproducing a simplified test for both cases and see if it makes a difference. – Yuval Itzchakov Dec 05 '15 at 15:23
@Saqib I'd be glad if you can elaborate if this made a steady difference in your tests. – Yuval Itzchakov Dec 05 '15 at 15:26
EAP methods usually keep themselves rooted, that's why that always works. If ServiceClients does not keep itself rooted I consider that a severe API design error/oversight. – usr Dec 05 '15 at 15:32
1

Some native component must eventually call back into native code to signal completion. That callback, to do its job, must use the object that started/owns the IO and access a delegate to call further callbacks and events. That pattern always automatically creates a rooting chain. The only special care that needs to be taken is to make sure the delegate that is passed to native code is not collected early. But this needs to be done regardless of rooting concerns simply for correctness reasons. So that decision is forced, too. – usr Dec 05 '15 at 15:35
I'm not a fan of fiddling with variables to try to create reference chains. In the end all of that is not guaranteed to work even if it works on the current CLR. Even static fields being roots is, I think, not guaranteed under ECMA. It is better to do things that work by principle, such as the IO pattern I just detailed. Or, using GC.KeepAlive. – usr Dec 05 '15 at 15:37
@usr How would you use `GC.KeepAlive` in the case of the OP? Would it even help with his `Process` method? – Yuval Itzchakov Dec 05 '15 at 15:39
I don't see how it could help here. If the entire graph is dead and no code is guaranteed to run in the future we have lost. GC.KeepAlive must be dynamically reachable to be guaranteed to have an effect. We would need GCHandle here. – usr Dec 05 '15 at 15:40
@usr I've added an edit to the answer which is a bit of a disclaimer for anyone who sees this answer in the future. – Yuval Itzchakov Dec 05 '15 at 15:45

score -2 · Answer 2 · answered Dec 05 '15 at 14:33

-2

I guess I should answer this.

public static Task<string> Process(Stream data)
{
    var handle = new AutoResetEvent(false);
    var client = new ServiceClient();
    var tcs = new TaskCompletionSource<string>();

    client.OnResult += (sender, e) =>
    {
        tcs.SetResult(e.Result);
        handle.Set();
    };

    client.OnError += (sender, e) =>
    {
        tcs.SetException(new Exception(e.ErrorMessage));
        handle.Set();
    };

    client.Send(data);

    handle.WaitOne(10000); // wait 10 secondds for results
    return tcs.Task;
}

answered Dec 05 '15 at 14:33

The Sharp Ninja

1,041
9
18

No, the await keyword does just that. It waits for the background task to finish. If you don't want to wait, call your Process method without the await keyword. – The Sharp Ninja Dec 05 '15 at 14:57
1

I meant the use of WaitOne which will prevent the method from returning for 10 seconds. `await` does not come into play until Process returns. – usr Dec 05 '15 at 15:22
You miss the point. If you use await, then you are blocking the thread calling Process. Performing a block in the thread does not prevent the UI from working if you don't use await! However, since you need the string, and the string has to be waited on, you really don't have a valid use case for arguing about blocking. – The Sharp Ninja Dec 05 '15 at 15:32
1

@TheSharpNinja The fact you think `await` blocks a thread shows you don't understand async\await. – Daniel Kelley Dec 07 '15 at 08:53

Wrapping a library that uses the Event-Based Asyncronous Pattern, for use with Async/Await

2 Answers2