-
Notifications
You must be signed in to change notification settings - Fork 171
Fixed crash in RabbitMQ connector #1569
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
Conversation
9119fdf to
a6bdbab
Compare
|
This bug was introduced in #1459. |
a6bdbab to
2cb6354
Compare
> System.InvalidCastException: 'Unable to cast object of type 'AsyncStateMachineBox`1[System.IDisposable,Steeltoe.Connectors.RabbitMQ.RabbitMQServiceCollectionExtensions+<CreateConnectionAsync>d__5]' to type 'RabbitMQ.Client.IConnection'.' This happened because the connect callback returned a `Task<IConnection>`, whereas the connector is registered in the service container with `IConnection`. There are two solutions: 1. Change the service registration from `ConnectorFactory<RabbitMQOptions, IConnection>` to `ConnectorFactory<RabbitMQOptions, Task<IConnection>>`, such that the call to `connectorFactory.Get().GetConnection()` needs to be awaited. 2. Stick with the current API and synchronously wait for the task to complete before returning it. This PR implements solution 2, because that doesn't require breaking the public API. I'm also unsure whether it's safe when the task is awaited concurrently.
2cb6354 to
ee8c7b8
Compare
Summary - All Code Coverage (ubuntu-latest)
|
|
Sticking with the current API seems like the right thing to do, this approach seems right given where we're at with 4.0 and nothing jumps out at me as blockers for merge, but I have a couple questions before approving:
|
Yes, that's an anti-pattern. See https://www.rabbitmq.com/dotnet-api-guide.html#connection-and-channel-lifespan.
That's basically solution 1, but with a different method name. I'm not sure that awaiting concurrently is thread-safe. |
TimHess
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM



Description
Fixed crash during RabbitMQ connect:
This happened because the connect callback returned a
Task<IConnection>, whereas the connector is registered in the service container withIConnection. There are two solutions:ConnectorFactory<RabbitMQOptions, IConnection>toConnectorFactory<RabbitMQOptions, Task<IConnection>>, such that the call toconnectorFactory.Get().GetConnection()needs to be awaited.This PR implements solution 2, because that doesn't require breaking the public API. I'm also unsure whether it's safe when the task is awaited concurrently.
Quality checklist
If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.