Skip to content

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