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] Improved Connection Manager #305

Merged
merged 40 commits into from
Mar 2, 2023
Merged

[network] Improved Connection Manager #305

merged 40 commits into from
Mar 2, 2023

Conversation

0xcb9ff9
Copy link

@0xcb9ff9 0xcb9ff9 commented Feb 15, 2023 •
edited
Loading

Description

Issues:

  • identity protocol stream is never closed, IdentityService cannot save the identity protocol stream during the handshake.
  • if bootnode shutdown (like network interruption), never re-connect known peers
  • PeerConnInfo not match the libp2p connection

This PR improved connection manager, key point:

  • wrap grpc client, use runtime.SetFinalizer cleanup
  • cleanup connect by background task
  • save known peer address info, random re-connect

Changes include

  • Bugfix (non-breaking change that solves an issue)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels

Testing

  • I have tested this code with the official test suite

@0xcb9ff9 0xcb9ff9 added the bug fix Functionality that fixes a bug label Feb 15, 2023
@0xcb9ff9 0xcb9ff9 added this to the Release 1.3.0 milestone Feb 15, 2023
@0xcb9ff9 0xcb9ff9 self-assigned this Feb 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2023 •
edited
Loading

Codecov Report

Merging #305 (b28bbdc) into dev (3626f37) will increase coverage by 0.02%.
The diff coverage is 60.41%.

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

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev     #305      +/-   ##
==========================================
+ Coverage   47.76%   47.79%   +0.02%     
==========================================
  Files         131      132       +1     
  Lines       19867    20125     +258     
==========================================
+ Hits         9490     9618     +128     
- Misses       9558     9678     +120     
- Partials      819      829      +10
Impacted Files Coverage Δ
network/bootnodes.go 100.00% <ø> (+14.28%) ⬆️
network/gossip.go 84.31% <ø> (ø)
network/server_nonnetwork.go 0.00% <0.00%> (ø)
txpool/txpool.go 67.45% <0.00%> (ø)
network/discovery/discovery.go 27.74% <12.82%> (-4.99%) ⬇️
network/identity/identity.go 25.54% <24.00%> (+0.54%) ⬆️
protocol/client.go 49.21% <50.00%> (-0.40%) ⬇️
txpool/event_subscription.go 89.74% <55.55%> (-10.26%) ⬇️
network/keep_available.go 70.79% <70.79%> (ø)
network/server_discovery.go 73.18% <73.80%> (-1.45%) ⬇️
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DarianShawn
DarianShawn reviewed Feb 17, 2023
View reviewed changes
network/discovery/discovery.go Show resolved Hide resolved
network/server_discovery.go Outdated Show resolved Hide resolved
@DarianShawn DarianShawn changed the title [network] Fix stream connect leak WIP: [network] Fix stream connect leak Feb 17, 2023
@DarianShawn DarianShawn added the WIP work in process label Feb 17, 2023
@0xcb9ff9 0xcb9ff9 force-pushed the fix/identity-grpc-client-leak branch from 408fc55 to 525b25f Compare February 17, 2023 06:52
@0xcb9ff9 0xcb9ff9 changed the title WIP: [network] Fix stream connect leak WIP: [network] Fix grpc client connect leak Feb 20, 2023
@0xcb9ff9 0xcb9ff9 changed the title WIP: [network] Fix grpc client connect leak [network] Fix grpc client connect leak Feb 21, 2023
@0xcb9ff9 0xcb9ff9 removed the WIP work in process label Feb 21, 2023
@0xcb9ff9 0xcb9ff9 changed the title [network] Fix grpc client connect leak WIP: [network] Fix grpc client connect leak Feb 24, 2023
@0xcb9ff9 0xcb9ff9 added the WIP work in process label Feb 24, 2023
@0xcb9ff9 0xcb9ff9 changed the title WIP: [network] Fix grpc client connect leak [network] Fix grpc client connect leak Feb 27, 2023
@0xcb9ff9 0xcb9ff9 removed the WIP work in process label Feb 27, 2023
@0xcb9ff9 0xcb9ff9 changed the title [network] Fix grpc client connect leak [network] Improved Connection Manager Feb 27, 2023
DarianShawn
DarianShawn reviewed Feb 27, 2023
View reviewed changes
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Thanks for the great job. 🙌
Some questions here though.

network/wrappers/discovery.go Outdated Show resolved Hide resolved
network/discovery/discovery.go Outdated Show resolved Hide resolved
network/discovery/discovery.go Outdated Show resolved Hide resolved
network/wrappers/interface.go Outdated Show resolved Hide resolved
network/server.go Outdated Show resolved Hide resolved
network/server.go Outdated Show resolved Hide resolved
network/server.go Outdated Show resolved Hide resolved
network/server.go Show resolved Hide resolved
network/wrappers/identity.go Outdated Show resolved Hide resolved
txpool/txpool.go Show resolved Hide resolved
@0xcb9ff9 0xcb9ff9 added the WIP work in process label Feb 27, 2023
@0xcb9ff9 0xcb9ff9 changed the title [network] Improved Connection Manager WIP: [network] Improved Connection Manager Feb 27, 2023
@0xcb9ff9 0xcb9ff9 changed the title WIP: [network] Improved Connection Manager [network] Improved Connection Manager Mar 1, 2023
@0xcb9ff9 0xcb9ff9 removed the WIP work in process label Mar 1, 2023
@0xcb9ff9 0xcb9ff9 force-pushed the fix/identity-grpc-client-leak branch from eb532cb to b22dcfb Compare March 1, 2023 09:23
@0xcb9ff9 0xcb9ff9 mentioned this pull request Mar 1, 2023
Closed
5 tasks
@0xcb9ff9 0xcb9ff9 force-pushed the fix/identity-grpc-client-leak branch from b22dcfb to bb25322 Compare March 1, 2023 10:48
DarianShawn
DarianShawn reviewed Mar 1, 2023
View reviewed changes
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

Some issues left

network/identity/identity.go Show resolved Hide resolved
network/keep_available.go Outdated Show resolved Hide resolved
network/keep_available.go Show resolved Hide resolved
@0xcb9ff9 0xcb9ff9 force-pushed the fix/identity-grpc-client-leak branch from b28bbdc to be4991f Compare March 2, 2023 08:07
DarianShawn
DarianShawn approved these changes Mar 2, 2023
View reviewed changes
Copy link
Collaborator

@DarianShawn DarianShawn left a comment

Choose a reason for hiding this comment

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

LGTM 💯
Thanks for the contribution.

@DarianShawn DarianShawn merged commit d7c5a9e into dogechain-lab:dev Mar 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2023
@0xcb9ff9 0xcb9ff9 deleted the fix/identity-grpc-client-leak branch March 2, 2023 11:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DarianShawn DarianShawn DarianShawn approved these changes

@abrahamcruise321 abrahamcruise321 Awaiting requested review from abrahamcruise321 abrahamcruise321 is a code owner

Assignees

@0xcb9ff9 0xcb9ff9

Labels
bug fix Functionality that fixes a bug
Projects
None yet
Milestone
Release 1.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@0xcb9ff9 @codecov-commenter @DarianShawn