Open stebet opened 3 years ago
I like the proposal! Two things I'd like to have a discussion about:
Regadring the sync version, isn't it discouraged to call .GetAwaiter().GetResult()
? Link
I like the proposal! Two things I'd like to have a discussion about:
- OnBasicDeliverAsync has 7 parameters and is performance critical. Would it be beneficial to wrap them in a struct and pass by reference?
- ValueTask returns. I'm not the expert in this topic, but what benefits do we get by choosing the nongeneric valuetask over task for an API that mostly synchronously completed?
Regadring the sync version, isn't it discouraged to call
.GetAwaiter().GetResult()
? Link
ValueTask is a struct and allocates nothing in the synchronous case. No need to call GetAwaiter etc, since it has a .IsCompletedSuccessfully property so awaiting can be skipped.
It's explained well here: https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/
Yes, but Task doesn't allocate either in the sync case where nothing is returned. (Calls Task.CompletedTask) ValueTask contains multiple fields and is thus bigger than a reference, which makes it more expensive to return.
I completely get the need of ValueTask
To quote the article: "With the advent of enabling even asynchronous completions to be allocation-free, however, a non-generic ValueTask becomes relevant again. Thus, in .NET Core 2.1 we also introduced the non-generic ValueTask and IValueTaskSource. These provide direct counterparts to the generic versions, usable in similar ways, just with a void result."
Is my assumption correct that this is only beneficial if the non generic ValueTask is backed by a IValueTaskSource? Otherwise it's still allocating either way (Task & ValueTask)
Is my assumption correct that this is only beneficial if the non generic ValueTask is backed by a IValueTaskSource? Otherwise it's still allocating either way (Task & ValueTask)
Sort of, you can reuse ValueTasks and pool them.
Moving to 8.0 so that we can ship a 7.0 with a smaller set of breaking API changes if necessary.
I'd like to propose that the synchronous consumers/dispatchers be deprecated and removed from the 7.0 client in favor of asynchronous ones, to simplify the basic API.
Reasoning: Due to the asynchronous nature of message consumers, I think it doesn't make sense that consumers should have a synchronous interface. If the interface for consumers is asynchronous, they can still run synchronously if they do not require asynchrony by not utilizing the
await
keyword and returning cached tasks that are already completed.Simplified examples:
If someone would want to provide a synchronous dispatcher, the implementer would simply not use await, call whatever synchronous methods should be called and
return default;
which in the case of ValueTask is a task that is simply completed. If the main client simply checks if the dispatcher method ran synchronously before awaiting, they automatically become synchronous and no task is required.This is in-line with how .NET Framework/Core uses pluggable handlers in HttpClient.