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

Fix disconnect race. #2088

Open
wants to merge 4 commits into
base: unstable
Choose a base branch
from
Open

Fix disconnect race. #2088

wants to merge 4 commits into from

Conversation

cheatfate
Copy link
Contributor

  • Fix PeerPool to use getFuture() only once when adding peer.
  • Add isAlive(peer) procedure.
  • Add join(peer) procedure.
  • Remove useless metric (discovery).
  • Add notifyAndWait() procedure which will help to avoid race while disconnecting.

@arnetheduck arnetheduck added the 🕤 Postponed Not scheduled for the current sprint label Nov 29, 2020
@cheatfate cheatfate marked this pull request as draft January 7, 2021 10:57
@cheatfate cheatfate changed the base branch from devel to unstable January 7, 2021 11:01
@cheatfate cheatfate marked this pull request as ready for review January 7, 2021 12:45
@tersec tersec removed the 🕤 Postponed Not scheduled for the current sprint label Jan 13, 2021
# All the `peer.disconnectedFut` callbacks are already scheduled in current
# `poll()` call, to avoid race we going to finish only in next `poll()`
# call.
callSoon(continuation, cast[pointer](retFuture))
Copy link
Member

Choose a reason for hiding this comment

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

the pointer isn't needed here

Add isAlive(peer) procedure.
Add join(peer) procedure.
Remove useless metric.
Add notifyAndWait() procedure which will help to avoid race while disconnecting.
notifyAndWait() now waits PeerPool and disconnection.
@github-actions
Copy link

github-actions bot commented Nov 18, 2021

Unit Test Results

     12 files  ±0     744 suites  ±0   33m 19s ⏱️ +26s
1 467 tests +1  1 465 ✔️ +1  2 💤 ±0  0 ±0 
8 936 runs  +4  8 928 ✔️ +4  8 💤 ±0  0 ±0 

Results for commit 4957ae8. ± Comparison against base commit f19a497.

♻️ This comment has been updated with latest results.

block:
var fut0 = pool.joinPeer(peer0)
doAssert(fut0.finished == false)
await sleepAsync(20.milliseconds)
Copy link
Member

Choose a reason for hiding this comment

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

can't we use the step polling here instead?

if not(isNil(fut)):
fut.complete()
peer.disconnectedFut = nil
if not(isNil(peer.disconnectedFut)):
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this check be inside notifyAndWait?

# All the `peer.disconnectedFut` callbacks are already scheduled in current
# `poll()` call, to avoid race we going to finish only in next `poll()`
# call.
callSoon(continuation, cast[pointer](retFuture))
Copy link
Member

Choose a reason for hiding this comment

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

continuation does not use its udata pointer - why include a pointer here?

@arnetheduck arnetheduck force-pushed the unstable branch 2 times, most recently from 657f9d5 to a4667d1 Compare January 6, 2022 16:14
Base automatically changed from unstable to stable February 15, 2022 20:57
@tersec tersec changed the base branch from stable to unstable March 21, 2022 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants