-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
rpcclient: safe read and write to batch #2273
Conversation
Approved CI. |
Pull Request Test Coverage Report for Build 12370739640Details
💛 - Coveralls |
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.
Thanks for the fix! Just two minor nits.
One more nit here b3d9947 |
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.
Can you squash the commits into one please?
Then this is good to go.
a8cddef
to
d815651
Compare
Could you do a rebase on master? I'm trying to test it in |
d815651
to
db5318b
Compare
done |
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 🚢
This PR tries to make batchList concurrent-safe.
The
Send()
could panic at the line 1730 if c.removeRequest returnednil
. It could returnnil
if we try to callSend()
concurrently.btcd/rpcclient/infrastructure.go
Lines 1708 to 1730 in 684d64a
After the quick look I've noticed that there are places where the batchList is update unsafely. So, the simplest fix is just to add locks, but I'm afraid if it could break something else, because I'm not very familiar with the library.