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

GossipSub: remove peer if we can't establish sendConn #886

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

Menduist
Copy link
Contributor

@Menduist Menduist commented May 5, 2023

Currently, if we can't establish a sendConn, we will swallow messages, which can lead to asymmetric mesh and useless memory usage
This PR changes the behavior to consider the peer disconnected if we can't establish a sendConn in 5 seconds

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #886 (0f73ce0) into unstable (a1eb53b) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable     #886      +/-   ##
============================================
- Coverage     84.28%   84.27%   -0.01%     
============================================
  Files            91       91              
  Lines         15322    15323       +1     
============================================
  Hits          12914    12914              
- Misses         2408     2409       +1     
Impacted Files Coverage Δ
libp2p/protocols/pubsub/pubsubpeer.nim 87.91% <100.00%> (-0.60%) ⬇️

... and 5 files with indirect coverage changes

@Menduist Menduist marked this pull request as ready for review June 12, 2023 08:15
@diegomrsantos
Copy link
Collaborator

how about tests?

@Menduist
Copy link
Contributor Author

This PR is very hard to test because it only happens with buggy clients

@Menduist Menduist merged commit a65b7b0 into unstable Jun 14, 2023
@Menduist Menduist deleted the gossipsubconntimeout branch June 14, 2023 15:23
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.

2 participants