Skip to content

Expose DispatchConsumersAsync as a readonly from ConnectionFactory to IConnection #1611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

luizcarlosfaria
Copy link
Contributor

@luizcarlosfaria luizcarlosfaria commented Jun 23, 2024

Proposed Changes

Expose DispatchConsumersAsync property from internal _config.DispatchConsumersAsync to a read only property on IConnection interface, described in #1610.

Creating a consumer that receive a IConnection from dependency injection, without touch on IConnectionFactory, understand if connection will use async or sync dispatch is important to validate and help a developer to configure ConnectionFactory right.

It will enable to create a validation that avoids starting a consumer who never dispatches messages to user code because the connection is defined with synchronous dispatch and consumer is asynchronous or vice versa.

Types of Changes

Promote a hidden property defined on constructor to expose as a part of IConnection Interface.

  • New feature (non-breaking change which adds functionality)

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

@pivotal-cla
Copy link

@luizcarlosfaria Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@luizcarlosfaria Thank you for signing the Contributor License Agreement!

Removing trailing lines
@michaelklishin
Copy link
Contributor

Thank you for taking the time to contribute.

@lukebakken lukebakken self-requested a review June 25, 2024 21:50
@lukebakken lukebakken self-assigned this Jun 25, 2024
@lukebakken lukebakken modified the milestones: 8.0.0, 7.0.0 Jun 25, 2024
@lukebakken
Copy link
Collaborator

Hello and thanks for your contribution. Since version 7 should ship "soon", I'm hesitant to accept API changes.

Please note that using the wrong combination of DispatchConsumersAsync and a consumer class will throw an exception:

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/client/impl/ChannelBase.cs#L976-L982

@lukebakken lukebakken closed this Jun 25, 2024
@lukebakken lukebakken removed this from the 7.0.0 milestone Jun 25, 2024
@luizcarlosfaria
Copy link
Contributor Author

luizcarlosfaria commented Jun 27, 2024

Hello and thanks for your contribution. Since version 7 should ship "soon", I'm hesitant to accept API changes.

Please note that using the wrong combination of DispatchConsumersAsync and a consumer class will throw an exception:

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/client/impl/ChannelBase.cs#L976-L982

Great news, but instead, must be adding an inverse check to guarantee that if ConsumerDispatcher is Synchronous, throw a exception when consumer is async.

In my case, the only implementation of consumer is Async, but sometime, users do not configure ConnectionFactory properly, ignoring DispatchConsumersAsync property (by default is false).

The way to make consistence guarantee is requesting the user ConnectionFactory to do my validations but by design handle the ConnectionFactory is not my business.

ConnectionFactory and Connection is handled by user code, my job is to use a valid connection to create a consumer.

The effect is that consumer never be called.

@lukebakken
Copy link
Collaborator

Hi @luizcarlosfaria, I didn't realize you're using this library as part of a larger project that could benefit RabbitMQ users everywhere (https://github.com/luizcarlosfaria/Oragon.RabbitMQ).

Let me re-visit this PR. I agree, the validation needs to be consistent.

lukebakken added a commit that referenced this pull request Jun 27, 2024
* Name new property `DispatchConsumersAsyncEnabled`.
* Add a check on `BasicConsume` for when a regular dispatcher is used, and an async consumer passed.
lukebakken pushed a commit that referenced this pull request Jun 27, 2024
* Name new property `DispatchConsumersAsyncEnabled`.
* Add a check on `BasicConsume` for when a regular dispatcher is used, and an async consumer passed.
* Test the new `DispatchConsumersAsyncEnabled` property.
@lukebakken
Copy link
Collaborator

I have taken the code in this PR and added to it here - #1615

@lukebakken lukebakken closed this Jun 27, 2024
lukebakken added a commit that referenced this pull request Jun 27, 2024
…llowup

Add `DispatchConsumersAsyncEnabled` property on `IConnection` (#1611)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants