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.

Sign up for GitHub

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

Jump to bottom

Network conn overflow and shutdown #691

Merged
merged 3 commits into from
Feb 28, 2020
Merged

Network conn overflow and shutdown #691

merged 3 commits into from
Feb 28, 2020

Conversation

roman-khimov
Copy link
Member

Problem

Deadlocks on connection overflow and server shutdown.

Solution

Merge button.

@roman-khimov
network: don't deadlock on connection overflow
e2116e4 
(*Peer).Disconnect send an unregister signal to this goroutine, so invoking it
from here is not a good idea, run it asynchronously.
@roman-khimov roman-khimov added the bug Something isn't working label Feb 24, 2020
@roman-khimov roman-khimov added this to the v0.74.0 milestone Feb 24, 2020
@roman-khimov roman-khimov force-pushed the network-conn-overflow-and-shutdown branch from 40ae09f to 4e0e7eb Compare February 24, 2020 14:16
@codecov
Copy link

codecov bot commented Feb 24, 2020 •
edited
Loading

Codecov Report

Merging #691 into master will increase coverage by 0.03%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
+ Coverage   65.01%   65.05%   +0.03%     
==========================================
  Files         125      125              
  Lines       10820    10833      +13     
==========================================
+ Hits         7035     7047      +12     
- Misses       3505     3506       +1     
  Partials      280      280
Impacted Files Coverage Δ
pkg/network/discovery.go 99.05% <100%> (+0.12%) ⬆️
pkg/network/server.go 16.73% <20%> (-0.04%) ⬇️

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 cad1f07...ccffe2f. Read the comment docs.

roman-khimov
roman-khimov commented Feb 24, 2020
View reviewed changes
pkg/network/discovery.go
@@ -34,6 +35,7 @@ type DefaultDiscovery struct {
connectedAddrs map[string]bool
goodAddrs map[string]bool
unconnectedAddrs map[string]int
isDead bool
Copy link
Member Author

Choose a reason for hiding this comment

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

Not the most elegant solution, but with the current design I'm not sure anything better is possible (absent this kludge we'll panic on RequestRemote after Close).

fyrchik
fyrchik suggested changes Feb 26, 2020
View reviewed changes
pkg/network/discovery.go Outdated Show resolved Hide resolved
pkg/network/discovery.go Outdated Show resolved Hide resolved
@roman-khimov roman-khimov force-pushed the network-conn-overflow-and-shutdown branch from 4e0e7eb to ccffe2f Compare February 27, 2020 14:10
@codecov
Copy link

codecov bot commented Feb 27, 2020 •
edited
Loading

Codecov Report

Merging #691 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
+ Coverage   65.01%   65.04%   +0.02%     
==========================================
  Files         125      125              
  Lines       10820    10835      +15     
==========================================
+ Hits         7035     7048      +13     
- Misses       3505     3507       +2     
  Partials      280      280

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 cad1f07...e213e69. Read the comment docs.

@roman-khimov roman-khimov force-pushed the network-conn-overflow-and-shutdown branch from ccffe2f to 815436c Compare February 28, 2020 12:55
@roman-khimov
network: add Close() to discoverer, shut it down on exit
77624a8
@roman-khimov
network: rework shutdown sequence of Server
e213e69 
Close transport and disconnect peers right in the Shutdown(), so that no new
connections would be accepted and so that all the peers would be disconnected
correctly (avoiding the same deadlock as in e2116e4).
@roman-khimov roman-khimov force-pushed the network-conn-overflow-and-shutdown branch from 815436c to e213e69 Compare February 28, 2020 13:22
@roman-khimov roman-khimov merged commit 3619705 into master Feb 28, 2020
@roman-khimov roman-khimov deleted the network-conn-overflow-and-shutdown branch February 28, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fyrchik fyrchik fyrchik approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
v0.74.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@roman-khimov @fyrchik