Async all the way

[stebet]

Initial async implementation. Decided to put this up here and add some collaborators, instead of polluting the main RabbitMQ repo with a not-yet-completed, long-running PR. Also to spark some discussions on how feasible this is as a major change, or if it would simply make more sense to release a completely different package with the async version due to the amount of breaking changes involved.

List of interesting changes:
ModelBase.cs - https://github.com/stebet/rabbitmq-dotnet-client/pull/3/files#diff-b5b88775d543e4647d83cd30a6909bd1
SocketFrameHandler.cs - https://github.com/stebet/rabbitmq-dotnet-client/pull/3/files#diff-d56528b89584ed384b8cd4125d4f2abd
Connection.cs - https://github.com/stebet/rabbitmq-dotnet-client/pull/3/files#diff-505de50e6370e0c153328803cd1a5db4
Frame.cs - https://github.com/stebet/rabbitmq-dotnet-client/pull/3/files#diff-4a1f7dd804ba9e80bcd236f26e37687d
RpcContinuationQueue.cs - https://github.com/stebet/rabbitmq-dotnet-client/pull/3/files#diff-7fde3385e9c372a542f70b25dc2f7429

Current allocation statistics from my benchmark app (down from 65mb -> 49mb) compared to data in rabbitmq#855:

[stebet]

Currently, this is a pretty-complete ValueTask-based async-implementation of the RabbitMQ client. Allocations have been reduced even further, and some pretty hefty performance gains as well in my tests at least. This currently compiles and almost all tests run (one fails which might be obsolete with an async client) but there is some cleanup to be done yet.

[stebet]

I have yet to write up a bigger description of the functional changes although public API changes other than operations being async are minimal. This takes advantage of System.IO.Pipelines and targets netstandard2.1 but could possibly be converted to target netstandard2.0 if we can get our hands on ManualResetValueTaskCore which might be available in the Microsoft.BCL.AsyncInterfaces package.

[stebet]

and targets netstandard2.1 but could possibly be converted to target netstandard2.0 if we can get our hands on ManualResetValueTaskCore which might be available in the Microsoft.BCL.AsyncInterfaces package.

Made this work :) Now targets .NET Standard 2.0

[danielmarbach]

Need to look in more details. One initial question though:

What's the reason you went with ValueTask as default?

Should every new asynchronous API return ValueTask / ValueTask?

In short, no: the default choice is still Task / Task.

As highlighted above, Task and Task are easier to use correctly than are ValueTask and ValueTask, and so unless the performance implications outweigh the usability implications, Task / Taskare still preferred. There are also some minor costs associated with returning a ValueTask instead of a Task, e.g. in microbenchmarks it’s a bit faster to await a Task than it is to await a ValueTask, so if you can use cached tasks (e.g. you’re API returns Task or Task), you might be better off performance-wise sticking with Task and Task. ValueTask / ValueTask are also multiple words in size, and so when these are awaitd and a field for them is stored in a calling async method’s state machine, they’ll take up a little more space in that state machine object.

However, ValueTask / ValueTask are great choices when a) you expect consumers of your API to only await them directly, b) allocation-related overhead is important to avoid for your API, and c) either you expect synchronous completion to be a very common case, or you’re able to effectively pool objects for use with asynchronous completion. When adding abstract, virtual, or interface methods, you also need to consider whether these situations will exist for overrides/implementations of that method.

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/

Isn't most of the public API here more a candidate for Task instead of ValueTask should we want to follow those recommendations?

[stebet]

Need to look in more details. One initial question though:

What's the reason you went with ValueTask as default?

...

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/

Isn't most of the public API here more a candidate for Task instead of ValueTask should we want to follow those recommendations?

Quite right, a lot of it is. And that's one of the things to look at in more detail. There are conflicting views on this however: https://blog.marcgravell.com/2019/08/prefer-valuetask-to-task-always-and.html and @mgravell makes a lot of good points there.

It's an interesting dilemma. Basically, whenever you might have an async method execute synchronously (obviously the RPC methods should never do that, but a lot of the System.IO.Pipelines based serialization/flushing does) you'd want to use a ValueTask. Also ValueTasks are more easily reusable to reduce allocations than Tasks (via. ManualResetValueTaskSourceCore). Consumers should be none-the-wiser really, unless they actually are awaiting more than once, which should really be a no-no, but that's me making a lot of assumptions.

I have yet to do benchmarks and tests on what happens if we change the public facing async methods to Task (where appropriate).

[stebet]

B.t.w, we could also PR this into a separate branch on the main repo if you'd like to get this code there for further work/review in a more official way @lukebakken @michaelklishin.

[lukebakken]

@stebet that's a great idea. Talk about a lot of work here! I've added you as a contributor to the main repo so feel free to start a branch there with these changes.

[stebet]

@stebet that's a great idea. Talk about a lot of work here! I've added you as a contributor to the main repo so feel free to start a branch there with these changes.

Will do :)

[stebet]

Rebased and squashed the changes before initial PR to new branch.