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(share/discovery): timeout FindPeers operation #2263

Merged
merged 8 commits into from
May 30, 2023
Merged

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented May 26, 2023

Closes #2258

FindPeers can get stuck sometimes for an indefinite time and so forth, stopping the whole discovery. We should stop and restart it.

@Wondertan Wondertan added area:shares Shares and samples kind:fix Attached to bug-fixing PRs labels May 26, 2023
@Wondertan Wondertan self-assigned this May 26, 2023
@Wondertan
Copy link
Member Author

@walldiss, pls test to reconfirm the fix works

walldiss
walldiss previously approved these changes May 26, 2023
share/p2p/discovery/discovery.go Outdated Show resolved Hide resolved
share/p2p/discovery/discovery.go Outdated Show resolved Hide resolved
share/p2p/discovery/metrics.go Outdated Show resolved Hide resolved
share/p2p/discovery/metrics.go Show resolved Hide resolved
@walldiss
Copy link
Member

I have run node with this PR and never seen stuck discovery since then

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2023

Codecov Report

Merging #2263 (8b58eed) into main (009d5a9) will increase coverage by 0.00%.
The diff coverage is 64.28%.

❗ Current head 8b58eed differs from pull request most recent head a4a8b1d. Consider uploading reports for the commit a4a8b1d to get more accurate results

@@           Coverage Diff           @@
##             main    #2263   +/-   ##
=======================================
  Coverage   50.44%   50.44%           
=======================================
  Files         150      150           
  Lines        9318     9294   -24     
=======================================
- Hits         4700     4688   -12     
+ Misses       4217     4204   -13     
- Partials      401      402    +1     
Impacted Files Coverage Δ
share/p2p/discovery/metrics.go 11.53% <50.00%> (+1.45%) ⬆️
share/p2p/discovery/discovery.go 68.96% <66.66%> (-3.24%) ⬇️

... and 2 files with indirect coverage changes

walldiss
walldiss previously approved these changes May 29, 2023
@Wondertan Wondertan enabled auto-merge (squash) May 29, 2023 12:57
@Wondertan Wondertan changed the title fix(share/discovery): memoize discovery trigger and limit FindPeers operation fix(share/discovery): timeout FindPeers operation May 30, 2023
@Wondertan
Copy link
Member Author

We discussed in sync that we don't want to merge the fix, we are not sure it is fixing anything, so we removed the buffer. We can continue testing to see if the discovery still gets stuck

@Wondertan Wondertan merged commit 77d511d into main May 30, 2023
@Wondertan Wondertan deleted the hlib/discovery-fix branch May 30, 2023 08:35
Wondertan added a commit that referenced this pull request May 30, 2023
…peration (#2263)

Closes #2258

Two cases were possible:
* Sometimes, the discovery is not triggered, and memorizing triggers might help
	* It's an unconfirmed theory, and we are still determining if it fixes anything yet.
	* We considered a case with @walldiss, but it should not happen.
* FindPeers can get stuck sometimes for an indefinite time and so forth stopping the whole discovery. We should stop and restart it.
github-merge-queue bot pushed a commit that referenced this pull request Jun 21, 2023
Closes #2263

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: discovery cannot find peers after network disconnect
5 participants