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 Swarm.DialPeerAsync() #128

Merged
merged 2 commits into from Mar 13, 2019

Conversation

Projects
None yet
2 participants
@longfin
Copy link
Member

commented Mar 13, 2019

In failed test, Swarm.DialPeerAsync() seems to add unreachable peer.

        05:59:47[@8eFB6385bAB28Fc189D4692333b8926AFb23Fa3c][13] - TimeoutException occured in DialAsync (127.0.0.1:50040).
        05:59:47[@8eFB6385bAB28Fc189D4692333b8926AFb23Fa3c][13] - DialPeerAsync(0x43056260567c13B4Dce2b3ac6F6F6a86ecE5EB8E.127.0.0.1:50040) is complete.
        05:59:47[@8eFB6385bAB28Fc189D4692333b8926AFb23Fa3c][13] - 3/13/2019 5:59:30 AM +00:00 0x43056260567c13B4Dce2b3ac6F6F6a86ecE5EB8E.127.0.0.1:50040 > +0x43056260567c13B4Dce2b3ac6F6F6a86ecE5EB8E.127.0.0.1:50040
        05:59:47[@8eFB6385bAB28Fc189D4692333b8926AFb23Fa3c][13] - 3/13/2019 5:59:30 AM +00:00 0x43056260567c13B4Dce2b3ac6F6F6a86ecE5EB8E.127.0.0.1:50040 > -0x43056260567c13B4Dce2b3ac6F6F6a86ecE5EB8E.127.0.0.1:50040
        05:59:47[@8eFB6385bAB28Fc189D4692333b8926AFb23Fa3c][12] - Trying to apply the delta[[Sender: 0x43056260567c13B4Dce2b3ac6F6F6a86ecE5EB8E.127.0.0.1:50040, +: 2, -: 0]]...
        05:59:47[@8eFB6385bAB28Fc189D4692333b8926AFb23Fa3c][12] - Trying to add peers...

this PR fixed Swarm.DialPeerAsync() to ignore properly.

@longfin longfin requested a review from dahlia Mar 13, 2019

e,
$"IOException occured in DialAsync ({peer.EndPoint}).");
dealer.Dispose();
throw e;
}

This comment has been minimized.

Copy link
@dahlia

dahlia Mar 13, 2019

Member

This catch condition seems able to be safely removed.

This comment has been minimized.

Copy link
@longfin

longfin Mar 13, 2019

Author Member

It's mistake. (dealer.Dispose() should be kept.) I'll fix it.

@longfin longfin force-pushed the longfin:bugfix/dial-peer-async branch from c09ad4b to 817904e Mar 13, 2019

@longfin longfin force-pushed the longfin:bugfix/dial-peer-async branch from 817904e to 78703a2 Mar 13, 2019

@longfin

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I've realized some other mistakes and fixed. PTAL @dahlia

@dahlia dahlia added the bug label Mar 13, 2019

@dahlia dahlia modified the milestone: 0.2.0 Mar 13, 2019

@dahlia

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

If the bug was there since 0.1.0 this pull request also should be merged to 0.1-maintenance. After this gets merged I will merge that back to master as well.

@longfin longfin changed the base branch from master to 0.1-maintenance Mar 13, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 13, 2019

Codecov Report

Merging #128 into master will decrease coverage by 0.16%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   88.18%   88.01%   -0.17%     
==========================================
  Files          61       61              
  Lines        2987     2988       +1     
==========================================
- Hits         2634     2630       -4     
- Misses        353      358       +5
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 90.05% <77.77%> (-0.67%) ⬇️
@dahlia

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Could you rebase only 2ed23f3 and 78703a2 on 0.1-maintenance? Other behind commits should be stripped.

@longfin

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

If the bug was there since 0.1.0 this pull request also should be merged to 0.1-maintenance. After this gets merged I will merge that back to master as well.

it also exists in 0.1.0. but 0.1.0 and master don't have same patch set since they have different ways of handling the peer's endpoint.

I think 0.1.0 is not meaningful at this point, so I think it's also a way to solve this problem starting with 0.2.0.

@dahlia

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Ah, okay. It got a point.

@longfin longfin changed the base branch from 0.1-maintenance to master Mar 13, 2019

Fix Swarm.AddPeersAsync()
- ignore unreachable peer properly

@longfin longfin force-pushed the longfin:bugfix/dial-peer-async branch from 78703a2 to 184b501 Mar 13, 2019

@dahlia

dahlia approved these changes Mar 13, 2019

@longfin longfin merged commit 6ca8c00 into planetarium:master Mar 13, 2019

4 of 5 checks passed

codecov/patch 77.77% of diff hit (target 88.18%)
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/project 88.01% (-0.17%) compared to e7e56e4
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.