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 broadcasting #218

Merged
merged 19 commits into from
May 9, 2019
Merged

Conversation

longfin
Copy link
Member

@longfin longfin commented Apr 25, 2019

This PR changes the broadcasting mechanism from TAP to NetMQPoller based to prevent unexpected behavior on multi threading environment. to accomplish it, it made some interface changes that convert some asynchronous methods to sync methods.

Also, it adds time out on the unit tests to investigate the test freezing and fixes some problems in CI environment.

@longfin longfin force-pushed the feature/swarm-broadcast branch 3 times, most recently from 191beee to a7b489d Compare April 26, 2019 05:53
@longfin longfin marked this pull request as ready for review April 26, 2019 05:54
@longfin longfin changed the title [WIP] Fix Swarm broadcasting Fix Swarm broadcasting Apr 26, 2019
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@@ -87,6 +91,7 @@ public class Swarm : ICollection<Peer>, IDisposable
PrivateKey privateKey,
int appProtocolVersion,
TimeSpan dialTimeout,
TimeSpan linger,
Copy link
Contributor

Choose a reason for hiding this comment

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

A naming suggestion: how about gracePeriod?

Copy link
Member Author

Choose a reason for hiding this comment

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

gracePeriod is also good, but I think linger is general enough to explain this behavior.

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
earlbread
earlbread previously approved these changes Apr 26, 2019
@longfin longfin force-pushed the feature/swarm-broadcast branch 12 times, most recently from 2800642 to 25e357f Compare April 30, 2019 04:38
@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #218 into master will decrease coverage by 0.19%.
The diff coverage is 87.5%.

@@           Coverage Diff            @@
##           master    #218     +/-   ##
========================================
- Coverage    84.1%   83.9%   -0.2%     
========================================
  Files          75      75             
  Lines        3491    3504     +13     
========================================
+ Hits         2936    2940      +4     
- Misses        555     564      +9
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 79.86% <87.5%> (-0.72%) ⬇️

@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #218 into master will decrease coverage by 0.08%.
The diff coverage is 82.66%.

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage    84.1%   84.02%   -0.09%     
==========================================
  Files          75       75              
  Lines        3492     3505      +13     
==========================================
+ Hits         2937     2945       +8     
- Misses        555      560       +5
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 80.39% <82.66%> (-0.19%) ⬇️
Libplanet/Blockchain/BlockChain.cs 98.01% <0%> (-0.03%) ⬇️

@longfin longfin force-pushed the feature/swarm-broadcast branch 4 times, most recently from 87fe742 to b912321 Compare May 7, 2019 04:26
earlbread and others added 2 commits May 8, 2019 11:42
Co-Authored-By: longfin <longfinfunnel@gmail.com>
Co-Authored-By: longfin <longfinfunnel@gmail.com>
@codecov-io
Copy link

codecov-io commented May 8, 2019

Codecov Report

Merging #218 into master will decrease coverage by 0.53%.
The diff coverage is 82.64%.

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage    84.1%   83.56%   -0.54%     
==========================================
  Files          75       75              
  Lines        3491     3511      +20     
==========================================
- Hits         2936     2934       -2     
- Misses        555      577      +22
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 78.93% <82.64%> (-1.66%) ⬇️
Libplanet/Net/NoSwarmContextException.cs 0% <0%> (-25%) ⬇️

@codecov-io
Copy link

Codecov Report

Merging #218 into master will decrease coverage by 0.19%.
The diff coverage is 87.5%.

@@           Coverage Diff            @@
##           master    #218     +/-   ##
========================================
- Coverage    84.1%   83.9%   -0.2%     
========================================
  Files          75      75             
  Lines        3491    3504     +13     
========================================
+ Hits         2936    2940      +4     
- Misses        555     564      +9
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 79.86% <87.5%> (-0.72%) ⬇️

@longfin
Copy link
Member Author

longfin commented May 8, 2019

I've add some commits to aid the unit tests. PTAL @dahlia @earlbread

@longfin longfin requested review from dahlia and earlbread May 8, 2019 08:12
CHANGES.md Outdated Show resolved Hide resolved
{
if (_removedPeers.ContainsKey(peer))
{
_removedPeers.Remove(peer);
}
}

var existingKeys = new HashSet<PublicKey>(
_peers.Keys.Select(p => p.PublicKey)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's unnecessary because there is no usage in this scope.

Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
@longfin longfin merged commit 49ac8da into planetarium:master May 9, 2019
limebell pushed a commit to limebell/libplanet that referenced this pull request Jul 7, 2021
OnedgeLee pushed a commit to OnedgeLee/libplanet that referenced this pull request Jan 31, 2023
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.

4 participants