Skip to content
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

txnsync: fix potential race during TestBasicCatchpointCatchup #3033

Merged

Conversation

tsachiherman
Copy link
Contributor

Summary

During fast catchup, we restart the transaction sync service very quickly.
This can cause a network message being sent, and the response would be returned to the "restarted" txnsync.

Since we don't want to disconnect the network connection itself ( which could have some messages enqueued ), the transaction sync would need to store the "returned" channel before sending the message. That would avoid the data race ( and safely ignore the incoming message ).

Test Plan

Use existing testing, and confirm against that.

@tsachiherman tsachiherman self-assigned this Oct 11, 2021
@tsachiherman tsachiherman requested a review from a user October 11, 2021 14:53
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #3033 (4b016de) into master (0d5b66e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3033      +/-   ##
==========================================
- Coverage   43.65%   43.63%   -0.02%     
==========================================
  Files         391      391              
  Lines       86764    86764              
==========================================
- Hits        37875    37861      -14     
- Misses      42861    42873      +12     
- Partials     6028     6030       +2     
Impacted Files Coverage Δ
txnsync/outgoing.go 92.85% <100.00%> (ø)
network/wsPeer.go 71.24% <0.00%> (-2.85%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
data/abi/abi_type.go 90.90% <0.00%> (-0.91%) ⬇️
ledger/acctupdates.go 66.92% <0.00%> (-0.29%) ⬇️
catchup/service.go 69.35% <0.00%> (ø)
data/transactions/verify/txn.go 44.29% <0.00%> (+0.87%) ⬆️
cmd/algoh/blockWatcher.go 80.95% <0.00%> (+3.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d5b66e...4b016de. Read the comment docs.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@tsachiherman tsachiherman merged commit 8568442 into algorand:master Oct 11, 2021
@tsachiherman tsachiherman deleted the tsachi/fix_txnsync_restart_data_race branch October 11, 2021 18:08
onetechnical pushed a commit that referenced this pull request Oct 25, 2021
## Summary

During fast catchup, we restart the transaction sync service very quickly.
This can cause a network message being sent, and the response would be returned to the "restarted" txnsync.

Since we don't want to disconnect the network connection itself ( which could have some messages enqueued ), the transaction sync would need to store the "returned" channel before sending the message. That would avoid the data race ( and safely ignore the incoming message ).

## Test Plan

Use existing testing, and confirm against that.
tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Nov 2, 2021
@egieseke egieseke mentioned this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants