-
Notifications
You must be signed in to change notification settings - Fork 140
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
Data race in the client example #72
Comments
The problem could be solved by guarding |
Hi @yaa110 go client.handleReconnect(addr) and try to publish immediately. Which actually returns an error without doing nothing... Because you are not checking the connection is opened or not if !client.isReady {
return errors.New("failed to push: not connected")
} |
Thank you @ykalayy. I will assume that this is due to user error. |
Um this is still a race condition in the example. The example is straight lifted from the example and so any changed that needed to be made per @ykalayy suggestion should be updated in the example. Now the suggestion to check if the client.isReady before calling Publish is still a race condition as both the go routine and and the lookup to isReady can be racey. Copying the EXACT example program and running it with race on shows it is racy. You can confirm at startup without a server, a startup with a server and when a server reconnects. Without Server
With Server.
|
I have also seen a lot of code in the wild based on the reconnect logic of this example (including inside my company) which has caused real issues so this example should be fixed. |
@nemith thanks for using this library and RabbitMQ. A suggestion - when reporting issues on GitHub, please save large amounts of text into a file to attach, or use the
By all means, contribute back to the RabbitMQ ecosystem and provide a pull request to fix the example code. It would be greatly appreciated. I will re-open this issue. |
Sorry about the formatting. I just followed the initial report in the issue and really i didn't think there was that much output but I could be a problem for people with smaller monitors and phones? Anyway that isn't the point. Back to the issue at hand I do think that part of the problem is that this library is extremely hard to use properly while still implementing autoreconnect that is actually race free and I still trying to figure out the best way to use it for my private project but if i find a good example I can submit a PR. Another idea is, for now, just remove all the auto-reconnect logic from the example to at least prevent copy and paste errors for others consuming this library. |
@nemith you'll be interested in this issue, #253, as well as @jxsl13 's project - https://github.com/jxsl13/amqpx I would like to have more time to dedicate to this project, but I'm currently working on supporting RabbitMQ customers with support contracts, as well as the next major release of the official .NET client. |
Some users rely on this example as a starting point to their applications. This commit fixes a data race that could cause issues in any code that relied on the example as base. Related to #72 Signed-off-by: Aitor Perez Cedres <[email protected]>
I submitted a PR #260 to address this issue. Any feedback/comments are welcome. |
Some users rely on this example as a starting point to their applications. This commit fixes a data race that could cause issues in any code that relied on the example as base. Related to #72 Signed-off-by: Aitor Perez Cedres <[email protected]>
Some users rely on this example as a starting point to their applications. This commit fixes a data race that could cause issues in any code that relied on the example as base. Related to #72 Signed-off-by: Aitor Perez Cedres <[email protected]>
How to reproduce
amqp.go
:Code Example
amqp_test.go
:-race
:go test -race ./...
Error Message
Test fails with the following message:
Environment
Go Version:
1.18.1-bullseye
Library version:
v1.3.4
The text was updated successfully, but these errors were encountered: