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

[network] grpc client passing timeout context #324

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

0xcb9ff9
Copy link

@0xcb9ff9 0xcb9ff9 commented Mar 17, 2023

Description

PR #323

Two questions

  1. When node A is getting blocks from node B, if node B is forced to shut down (or when the network connection is disconnected), GetBlocks will never expire, causing node A to block synchronously. Although difficult to reproduce, the problem does exist.

  2. PeerConnInfo.protocolClient saves the grpc client, but the timeout period can't be set in the libp2p stream. libp2p stream leaks if peer disconnects

This PR removes grpc client saving, passing timeout context, enforcing timeouts in libp2p streams

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
  • Use test environment dogechain-testenv , random reset TCP connection tested

@0xcb9ff9 0xcb9ff9 added the bug fix Functionality that fixes a bug label Mar 17, 2023
@0xcb9ff9 0xcb9ff9 self-assigned this Mar 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2023

Codecov Report

Merging #324 (c09b694) into dev (c23260d) will decrease coverage by 0.05%.
The diff coverage is 66.34%.

📣 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     #324      +/-   ##
==========================================
- Coverage   47.50%   47.46%   -0.05%     
==========================================
  Files         135      135              
  Lines       20444    20468      +24     
==========================================
+ Hits         9712     9715       +3     
- Misses       9898     9923      +25     
+ Partials      834      830       -4     
Impacted Files Coverage Δ
network/server_nonnetwork.go 0.00% <0.00%> (ø)
network/metrics.go 22.68% <30.55%> (+3.32%) ⬆️
protocol/client.go 50.76% <79.16%> (+2.15%) ⬆️
network/server.go 73.28% <91.30%> (+1.49%) ⬆️
network/discovery/discovery.go 26.95% <100.00%> (+0.42%) ⬆️
network/identity/identity.go 21.60% <100.00%> (ø)
network/server_discovery.go 76.25% <100.00%> (+2.31%) ⬆️
network/server_identity.go 91.75% <100.00%> (+0.35%) ⬆️

... and 2 files with indirect coverage changes

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

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.

Great to see this PR.
Only a little suggestion here.
By the way, do we get any manual tests to distinguish the current fix from elder version?

network/client/metrics.go Show resolved Hide resolved
@DarianShawn
Copy link
Collaborator

@0xcb9ff9 The working commit is 4d60cdc, right?

DarianShawn
DarianShawn previously approved these changes Mar 21, 2023
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

@0xcb9ff9
Copy link
Author

yes, dogechain-testenv is a general test environment

DarianShawn
DarianShawn previously approved these changes Mar 24, 2023
@0xcb9ff9 0xcb9ff9 merged commit 7520391 into dogechain-lab:dev Apr 11, 2023
@0xcb9ff9 0xcb9ff9 deleted the fix/grpc-client branch April 11, 2023 02:06
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fix Functionality that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants