Make sure OnCallbackException is executed for AsyncConsumers

[BarShavit]

Proposed Changes

Always call to await when executing a task in AsyncConsumerWorkService. Previously, the wait called only if the job hasn't done yet. It lead to missing exception and missed the invoke of OnCallbackException event.

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 v6.2.1: CallbackException is not invoked for unhandled exceptions from AsyncEventingBasicConsumer.Received #944 )
• 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

[danielmarbach]

Nit 🔥 extra line

[bollhals]

Are you sure the tests are testing the correct thing?
(They are not very strict about what they verify the success of. Just that a exception was thrown.)

If I debug them, I get the exception thrown by throw new InvalidOperationException("Should never be called."); from AsyncDefaultBasicConsumer.cs

Also if I use the tests on master, they're green with or without your changes.

[danielmarbach]

@bollhals good catch. Honestly I was a bit lazy in reviewing the tests and mostly looked at the implementation side of things. Will take extra care next time (although I confess I was just about going on leave yesterday and I think that cause me to take shortcuts)

[BarShavit]

Copied the tests from the sync version.
Fixed that, now it fails without the changes.
The bug happened only if the registered received method is async. Also need to mark "DispatchConsumersAsync" as true, or it go to the default class and throws the exception throw new InvalidOperationException("Should never be called.");.

The task.delay(0) is just to avoid warnings (async without await in the method).

[bollhals]

I did some quick tests regarding performance of this change

Method	Mean	Error	StdDev	Median	Ratio
IsCompleted	23.24 ns	0.138 ns	0.202 ns	23.24 ns	1.00
Await	27.88 ns	0.055 ns	0.080 ns	27.86 ns	1.20
GetResult	23.62 ns	0.199 ns	0.297 ns	23.47 ns	1.02
RanToCompletion	25.08 ns	0.072 ns	0.106 ns	25.08 ns	1.08

So the currently suggested change seems like the most promising one!

[michaelklishin]

Thank you for sticking with it @BarShavit!

[bollhals]

This one can and should probably go back into 6.x

[michaelklishin]

Backported to 6.x.