Implement full async channel #982

bollhals · 2020-12-05T14:44:32Z

Proposed Changes

Implements the proposal (#970) except the wait for confirmation part (will be done later to keep this PR "smaller") and some smaller modification / renamings along the way.

Types of Changes

Bug fix (non-breaking change which fixes issue #NNNN)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause an observable behavior change in existing systems)
Documentation improvements (corrections, new content, etc)
Cosmetic change (whitespace, formatting, etc)

Checklist

I have read the CONTRIBUTING.md document
I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
All tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in related repositories

Further Comments

I've added xml documentation to all public API's
I've made the Channel classes as well as the interface public. The interface is returned in methods but it can be casted if users want to be able to.
I've extracted all channel 0 specific methods into the ConnectionChannel class, so the actual Channel is less crowded.
The implementation itself isn't fully async yet, but this is more about the API itself, so we can modify the things below next.

Open questions

Where should these types be saved in the source? (Current stored in "client/impl/channel")
What namespace should these types have?

projects/RabbitMQ.Client/client/FrameworkExtension/Interlocked.cs

bollhals · 2020-12-05T14:56:09Z

projects/RabbitMQ.Client/client/impl/CommandAssembler.cs

@@ -121,6 +121,12 @@ private void ParseHeaderFrame(in InboundFrame frame)
                throw new UnexpectedFrameException(frame.Type);
            }

+            if (totalBodyBytes == 0)


There was an exception thrown when ConsumerWorkService if the array was null (which was not caught due to an empty catch in the ConsumerWorkService, see other comment)

bollhals · 2020-12-05T14:57:05Z

projects/RabbitMQ.Client/client/impl/ConsumerWorkService.cs

                        {
-                            // ignored
+                            _channelBase.OnUnhandledExceptionOccurred(e, "ConsumerWorkService");


This is the empty catch I patched up

bollhals · 2020-12-05T14:59:12Z

projects/RabbitMQ.Client/util/SetQueue.cs

-
-namespace RabbitMQ.Util
-{
-    internal class SetQueue<T>


Was not used at all

bollhals · 2020-12-05T14:59:41Z

projects/Unit/DebugUtil.cs

@@ -34,7 +34,7 @@
 using System.IO;
 using System.Reflection;

-namespace RabbitMQ.Util
+namespace Unit


Was only used in tests, so I moved it there

bollhals · 2020-12-05T22:40:34Z

projects/RabbitMQ.Client/client/impl/Channel/Channel.cs

+        {
+            if (waitForConfirmation)
+            {
+                // TODO 8.0 - Call ModelRpc


I left various TODO 8.0 - in the code, this needs fixing in futher PRs

bollhals · 2020-12-05T22:43:35Z

Mind taking a look at this? @stebet @bording @michaelklishin
(You were commenting on #970 )

bollhals · 2020-12-10T21:55:13Z

I can't yet request formal reviews, but I would like to start having a discussion about these changes as they should mark the direction of 8.0.
@michaelklishin @stebet @danielmarbach @lukebakken

stebet · 2020-12-10T22:14:28Z

I can't yet request formal reviews, but I would like to start having a discussion about these changes as they should mark the direction of 8.0.
@michaelklishin @stebet @danielmarbach @lukebakken

I'll take a look at this tomorrow :)

danielmarbach

Sorry wasn't able to finish the whole thing. I have limited time at my hands before christmas. Here are a few thoughts

projects/RabbitMQ.Client/client/FrameworkExtension/Interlocked.cs

danielmarbach · 2020-12-17T07:27:37Z

projects/RabbitMQ.Client/client/api/AsyncDefaultBasicConsumer.cs

        ///  for use in acknowledging received messages, for instance.
        /// </summary>
-        public IModel Model { get; set; }
+        public IChannel Channel { get; set; }


I love how channel is better aligned with the purpose

projects/RabbitMQ.Client/client/api/IAsyncBasicConsumer.cs

projects/RabbitMQ.Client/client/api/IBasicConsumer.cs

projects/RabbitMQ.Client/client/api/IConnection.cs

projects/RabbitMQ.Client/client/events/EventingBasicConsumer.cs

danielmarbach · 2020-12-17T07:46:31Z

projects/RabbitMQ.Client/client/impl/Channel/AutorecoveringChannel.cs

+            _disposed = true;
+        }
+
+        void IDisposable.Dispose()


I don't see much value in implementing both IDisposable as well as IAsyncDisposable. Especially not because the first one requires us to do evil sync over async. Let's embrace async properly in v8

We can think about dropping IDisposable, but there might be some drawbacks with it (e.g. DI container might not fully support it or 🤷‍♂️ )
I do not know what's the best way forward for this.

bollhals · 2020-12-17T21:18:59Z

Sorry wasn't able to finish the whole thing. I have limited time at my hands before christmas. Here are a few thoughts

Well it's a big change and any feedback is very much appreciated!

Regarding the questions about async in Consumer / Connection, as the PR was already quite huge, so I've tried to limit changes outside of the IModel/IChannel for a future PR. I think it's simple to do changes to full async all the way stepwise. But yes all of it should be modified!

michaelklishin · 2022-05-08T16:33:24Z

Sorry to drag you into resolving some non-trivial conflicts @bollhals. We decided to go ahead
and merge #1202 as it felt like the
right thing to do since we require net6.0 now for 7.0. This obviously affects all pull requests in flight.

lukebakken · 2022-05-13T14:05:07Z

I will hopefully be able to bring this PR up-to-date once #1199 is merged.

michaelklishin · 2022-05-15T12:07:17Z

I reverted #1202 because it makes back and forward-porting impossible unless all maintained branches adopt file-scoped namespaces.

lukebakken · 2023-03-01T22:21:16Z

@bollhals now that I've merged #1264 would you like to rebase this on main? I can give it a try as well.

bollhals · 2023-03-01T23:39:33Z

I'm swamped with other stuff right now, so I do not have a lot of time to invest in this. So if you/someone can take over, that would be great.

stebet · 2023-03-02T12:26:56Z

~~Would it make sense to make several small PRs, starting simply with ConnectAsync and then gradually going through the operations?~~

I'll take a quick look to see how much work this is to merge and go through before suggesting we take an approach like splitting it up into smaller PRs

lukebakken · 2023-03-02T17:17:17Z

Thanks @stebet

lukebakken · 2023-03-10T14:22:59Z

@stebet I'm starting on porting these changes to main, beginning with the IModel -> IChannel and related renames.

https://github.com/bollhals/rabbitmq-dotnet-client/tree/channel

Part 2 of #1308 - Port Interlocked.cs from #982

bollhals commented Dec 5, 2020

View reviewed changes

projects/RabbitMQ.Client/client/FrameworkExtension/Interlocked.cs Show resolved Hide resolved

bollhals commented Dec 5, 2020

View reviewed changes

projects/RabbitMQ.Client/util/SetQueue.cs

namespace RabbitMQ.Util

{

internal class SetQueue<T>

Copy link

Contributor Author

bollhals Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was not used at all

bollhals commented Dec 5, 2020

View reviewed changes

implement channel

1ea8c30

bollhals force-pushed the channel branch from 7fca66d to 1ea8c30 Compare December 5, 2020 22:17

bollhals commented Dec 5, 2020

View reviewed changes

danielmarbach reviewed Dec 17, 2020

View reviewed changes

michaelklishin changed the base branch from master to main June 24, 2021 14:35

lukebakken added this to the 7.0.0 milestone Mar 8, 2022

lukebakken changed the title ~~8.0 - implement full async channel~~ Implement full async channel Mar 8, 2022

lukebakken mentioned this pull request Mar 14, 2022

Add System.IO.Pipelines to be able to better support Async #949

Closed

11 tasks

lukebakken mentioned this pull request May 13, 2022

Release 7.0 RC1 #1056

Closed

lukebakken self-assigned this May 13, 2022

lukebakken closed this Mar 10, 2023

lukebakken mentioned this pull request Mar 10, 2023

Port "Implement Full Async Channel" PR to main #1308

Closed

3 tasks

lukebakken added a commit that referenced this pull request Mar 10, 2023

Port Interlocked.cs from #982

1d616ee

https://github.com/bollhals/rabbitmq-dotnet-client/tree/channel

lukebakken added a commit that referenced this pull request Mar 14, 2023

Merge pull request #1310 from rabbitmq/rabbitmq-dotnet-client-1308-2

d457b01

Part 2 of #1308 - Port Interlocked.cs from #982

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement full async channel #982

Implement full async channel #982

bollhals commented Dec 5, 2020

bollhals Dec 5, 2020 •

edited

Loading

bollhals Dec 5, 2020

bollhals Dec 5, 2020

bollhals Dec 5, 2020

bollhals Dec 5, 2020

bollhals commented Dec 5, 2020

bollhals commented Dec 10, 2020

stebet commented Dec 10, 2020

danielmarbach left a comment

danielmarbach Dec 17, 2020

danielmarbach Dec 17, 2020

bollhals Dec 17, 2020

bollhals commented Dec 17, 2020

michaelklishin commented May 8, 2022

lukebakken commented May 13, 2022

michaelklishin commented May 15, 2022

lukebakken commented Mar 1, 2023

bollhals commented Mar 1, 2023

stebet commented Mar 2, 2023 •

edited

Loading

lukebakken commented Mar 2, 2023

lukebakken commented Mar 10, 2023

Implement full async channel #982

Implement full async channel #982

Conversation

bollhals commented Dec 5, 2020

Proposed Changes

Types of Changes

Checklist

Further Comments

Open questions

bollhals Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

bollhals Dec 5, 2020

Choose a reason for hiding this comment

bollhals Dec 5, 2020

Choose a reason for hiding this comment

bollhals Dec 5, 2020

Choose a reason for hiding this comment

bollhals Dec 5, 2020

Choose a reason for hiding this comment

bollhals commented Dec 5, 2020

bollhals commented Dec 10, 2020

stebet commented Dec 10, 2020

danielmarbach left a comment

Choose a reason for hiding this comment

danielmarbach Dec 17, 2020

Choose a reason for hiding this comment

danielmarbach Dec 17, 2020

Choose a reason for hiding this comment

bollhals Dec 17, 2020

Choose a reason for hiding this comment

bollhals commented Dec 17, 2020

michaelklishin commented May 8, 2022

lukebakken commented May 13, 2022

michaelklishin commented May 15, 2022

lukebakken commented Mar 1, 2023

bollhals commented Mar 1, 2023

stebet commented Mar 2, 2023 • edited Loading

lukebakken commented Mar 2, 2023

lukebakken commented Mar 10, 2023

bollhals Dec 5, 2020 •

edited

Loading

stebet commented Mar 2, 2023 •

edited

Loading