Ability to do concurrent dispatches both on the async as well as the sync consumer

[danielmarbach]

Proposed Changes

An alternative to #806

If the interface is not extended then this can be added in a non breaking way on the level of the connection factory and later could be propagated to the interfaces in v7. I currently named the property ProcessingConcurrency to convey the intent for both the sync and the async case

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.

[danielmarbach]

@stebet what do you think about this spike

[danielmarbach]

This would introduce a capture of this

[stebet]

I'll take a look at this tomorrow :)

[stebet]

This looks good to me. I'm wondering if it'd be better to default AsyncConsumerConcurrency to Environment.ProcessorCount if it's enabled. That'd mean two properties though, AsyncConsumerConcurrencyEnabled (false when AsyncConsumerConcurrencyCount == 1, when set to true, would set AsyncConsumerConcurrencyCount = Environment.ProcessorCount if AsyncConsumerConcurrencyCount == 1).

Maybe that's just making things a bit convoluted perhaps... what do you think?

[danielmarbach]

My thought process was that Concurrency = 1 is just a special case of Concurrency = N that's why I went without having an additional bool, because then you only need to set it to something sensible which than automatically switches to fan out.

[danielmarbach]

I've pushed a few changes to the tests to make them more reliable.

[danielmarbach]

@michaelklishin @lukebakken any thoughts? This could go in 6.2 if you guys are willing to take it

[lukebakken]

I've been busy with other work. I'll try to get to this PR today. We have some bug fixes that are planned for 6.1.1 but it sounds like 6.2.0 should be the next release.

[danielmarbach]

No stress @lukebakken

[lukebakken]

Thanks @danielmarbach and @stebet for the review.

[danielmarbach]

@lukebakken where should we track whether this new property should be moved to the interface or not? I did deliberately not add it there to not add a breaking change. Ideally an issue that is added to the next major version with a breaking change label would be awesome

[lukebakken]

@danielmarbach we can create a new issue for that. I'm mid-review / testing of #868 / #878 right now 😄

[shaneqld]

Have BasicAck and BasicNack been made thread-safe yet? If not, users will run into strange exceptions (mostly protocol errors) when they work on highly concurrent workloads sans workarounds.

[stebet]

Have BasicAck and BasicNack been made thread-safe yet? If not, users will run into strange exceptions (mostly protocol errors) when they work on highly concurrent workloads sans workarounds.

Do you have a repro you can share? Would love to dig into if this is still a problem.

BasicAck and BasicNack are IModel operations, and it is not supported to share the same IModel instance between threads.

[danielmarbach]

I'd be interested to see and hear this as well. We are doing BasicAck and BasicNack since multiple versions concurrently. Is that something that has regressed recently?

[michaelklishin]

@shaneqld @danielmarbach @stebet concurrent acknowledgments of single deliveries are safe. Acknowledgments with multiple set to true can indeed cause protocol channel exceptions because double acking is an error in the protocol. But this can happen even with a single thread of execution. This is also not a novel problem, although I see that our doc section on concurrency of consumers does not cover this very well.

So I don't see how this approach changes anything for basic.ack and basic.{nack,reject} operations.

[danielmarbach]

BTW how can I contribute to the doco for the new feature I added. I don't want you guys to need to do all the grunt workAm 25.06.2020 17:49 schrieb Michael Klishin <notifications@github.com>: @shaneqld @danielmarbach @stebet concurrent acknowledgments of single deliveries are safe. Acknowledgments with multiple set to true can indeed cause protocol channel exceptions. But this can happen even with a single thread of execution. This is also not a novel problem, although I see that our doc section on concurrency of consumers does not cover this very well. So I don't see how this approach changes anything for basic.ack and basic.{nack,reject} operations. —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

[shaneqld]

I'd be interested to see and hear this as well. We are doing BasicAck and BasicNack since multiple versions concurrently. Is that something that has regressed recently?

I ran into this in a previous version with a different implementation of concurrency, but have been unable to reproduce in this latest version. When I have a moment I will update a project that has this workaround for the protocol error (and remove said workaround) and let you know if I run into any issues.