Sequence Number non-blocking

[danielmarbach]

Proposed Changes

Follow through on #1662

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• 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

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• 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

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

[lukebakken]

@danielmarbach if you have time to review my commit, that would be great. I figure now would be the time to make the event handlers for basic.ack, basic.nack and basic.return async as well.

This PR has a positive effect on the performance of the PublisherConfirms test application!

[danielmarbach]

I see what I can do over the weekend

[lukebakken]

Thanks, but don't worry about it until next week, really 😄 I've bumped the "hopeful" shipping date for v7 to the 20th.

[danielmarbach]

@lukebakken I reviewed it and basically that change opened a whole can of worms.

When I looked at the change, I started wondering why there are synchronous and non-synchronous event handlers and exception handlers now on the channel interface and wondered whether it makes sense to make them all async. This lead me then down the path or realizing the internal session interface event might also need to change which then has ripple effects on the connection events as well.

I think the change you pushed is unrelated to this PR and it is better to roll that commit back and have a dedicated follow-up PR (should you decide to continue pushing down that route).

A bunch of thoughts I had during the review of the changes:

• Why CallbackException and CallbackExceptionAsync. This is confusing and should be unified
• Why no FlowControlAsync, ChannelShutdown and RevoveryAsync
• TakeOver method seems not consistently implemented
• _callbackExceptionAsyncWrapper is not initialized
• Why is there no SessionShutdownAsync and corresponding CloseAsync nor NotifyAsync
• Why doesn't the connection provide event overloads? Assuming the API surface is async everywhere, it seems users of the client might do something async there too which requires them to do sync over async.
• Why are the Async Event Args not supporting a cancellation token similar to https://learn.microsoft.com/en-us/dotnet/api/azure.syncasynceventargs?view=azure-dotnet
• Why is the connection not supporting IAsyncDisposable?

To me, the API surface feels unfinished

[lukebakken]

Sure, I can revert it. I tried making all event handlers async a while back and it just plain didn't work.

[paulomorgado]

Sure, I can revert it. I tried making all event handlers async a while back and it just plain didn't work.

For async event handlers, you have to manually invoke and await every delegate in the invocation list. But it works.

[danielmarbach]

@paulomorgado That's already solved in the AsyncEventHandler and the AsyncEventHandlerWrapper in the code. It is just not followed through yet entirely. I'm happy to help out to address some of the comments I made above in another PR

[paulomorgado]

@paulomorgado That's already solved in the AsyncEventHandler and the AsyncEventHandlerWrapper in the code. It is just not followed through yet entirely. I'm happy to help out to address some of the comments I made above in another PR

The IAsyncDisposable should be fairly easy.

[lukebakken]

I will merge this as CI passes. We can deal with async events in a later version of this library (or, wait until someone demonstrates a need). Thanks everyone.

[lukebakken]

Thank you @danielmarbach !!!