Skip to content

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