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

Make sure each peer heartbeat has a timeout #2009

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 14, 2021

Motivation

It's hard to have consistent timeouts when we have to add a timeout() to the Ping and the completion channel during the heartbeat.

If the peer is busy, we want to wait for it to be ready, rather than closing the connection immediately (#1551).

Solution

  • cleanup the heartbeat code, so each heartbeat request/response runs in a future with a single timeout
  • just send the message to the peer, which will wait until the timeout if it is busy, then fail and close the connection

The code in this pull request has:

  • Documentation Comments
  • Integration Tests

Review

@dconnolly reviewed #1850, which this code is based on.

This change blocks some of the upcoming security refactors, so it should be merged soon.

Related Issues

Closes #1551.

Follow Up Work

Wait on shutdowns as well as the heartbeat timeout (#1783, #1678)

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium A-network Area: Network protocol updates or fixes labels Apr 14, 2021
@teor2345 teor2345 added this to the 2021 Sprint 7 milestone Apr 14, 2021
@teor2345 teor2345 requested a review from dconnolly April 14, 2021 01:06
@teor2345 teor2345 self-assigned this Apr 14, 2021
Also cleanup the heartbeat code, so each heartbeat request/response runs
in a future with a single timeout.
@mpguerra mpguerra removed this from the 2021 Sprint 7 milestone Apr 16, 2021
@teor2345 teor2345 added this to the 2021 Sprint 7 milestone Apr 19, 2021
@mpguerra mpguerra removed this from the 2021 Sprint 7 milestone Apr 19, 2021
@dconnolly dconnolly added this to the 2021 Sprint 7 milestone Apr 19, 2021
@dconnolly dconnolly merged commit 2cecd52 into ZcashFoundation:main Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make heartbeats wait for the connection queue to empty, with a timeout
3 participants