Review async RPC continuations, await, and ConfigureAwait

[lukebakken]

cc @paulomorgado @danielmarbach

Discussion about .ConfigureAwait(false) that prompted this issue:
#1426 (comment)

The current implementation of AsyncRpcContinuation<T> is such that the awaitable is configured in the class itself, here:

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/client/impl/AsyncRpcContinuations.cs#L72-L78

This means that await k does not use .ConfigureAwait(false):

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/client/impl/ChannelBase.cs#L256

The original implementation of AsyncRpcContinuation<T>.GetAwaiter() returned a TaskAwaiter<T>:

public TaskAwaiter<T> GetAwaiter()
{
    return _tcs.Task.GetAwaiter();
}

...however, even with the above code, await k does not allow .ConfigureAwait(false):

So, my questions are:

• Is the current implementation the correct way to use .ConfigureAwait(false)?
• If not, what is?

[paulomorgado]

One thing you must have in mind is that ConfigureAwait(_) configures the await and not the taks,

It needs to be wherever the await is,

[lukebakken]

@paulomorgado Yes I realize that, but in this case, with the original implementation of AsyncRpcContinuation<T>, configuring the await is not an option, which I find surprising.

[paulomorgado]

ConfigureAwait is an instance method of the Task and Task<TResult> classes and ValueTask and ValueTask<TResult> structs.

If k is not one of them, it doesn't have that method.

Check out the ConfigureAwait FAQ.

(I'm sorry, but I'm not on a device where I can check that).

[lukebakken]

Thank you @paulomorgado for the explanation.

Since AsyncRpcContinuation<T> instances are on the hot path, I'm assuming it makes sense to continue using ValueTask.

[lukebakken]

OK, I have modified AsyncRpcContinuation to have a WaitAsync() method rather than GetAwaiter() in this commit, which resolves this issue:

26696ef

[danielmarbach]

@lukebakken There was no issue, it was just that the previous version of the code didn't require ConfigureAwait(false) because it was already returning a correctly configured awaiter. A good analyzer would have caught that and not highlighted that you should add ConfigureAwait(false).

It is similar to Task.Yield. There, you cannot configure the context capturing.

[lukebakken]

Thanks for the clarification @danielmarbach. I thought the previous version was correct, but I think having an explicit WaitAsync may be a bit clearer.

[paulomorgado]

I think that should be left only for non-cancelable APIs. All other should already have a cancellation token and that one should be combined with a timeout cancellation token, when a timeout is required.