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

Simplify swarm interface #72

Merged
merged 5 commits into from
Feb 14, 2019

Conversation

longfin
Copy link
Member

@longfin longfin commented Feb 13, 2019

This PR simplifies Swarm's interface as #69. specifically, the following changes have been made.

  • Renamed RunAsync() / DisposeAsync() to StartAsync() / StopAsync()
  • Integrated InitContextAsync() into RunAsync()
  • Removed _delta queue and replace related logic with worker task.
  • Resolved ambiguous constructor overloading.
  • Made interval in StartAsync() to optional

@longfin longfin requested a review from dahlia February 13, 2019 13:14
@longfin longfin force-pushed the feature/simplify-swarm-interface branch from eb0ce6f to 357c3cd Compare February 13, 2019 13:15
await swarm.StopAsync();

Assert.False(swarm.Running);
await t;
Copy link
Contributor

Choose a reason for hiding this comment

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

We would be able (and indeed it's good) to test what if StopAsync() is called when it's not Running.

@@ -261,33 +227,32 @@ public void CopyTo(Peer[] array, int arrayIndex)
}
}

public async Task DisposeAsync(
public async Task StopAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

I predict that library users will tend to forget to call this after they started a swarm, in the similar way to forget fclose() in C. How about Swarm having a finalizer (i.e., ~Swarm()) and making it to call StopAsync() with a warning if it's Running? Python also does a similar thing for file objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since StopAsync() is an asynchronous function, I'm not sure if it can wait in the finalizer...

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Make sense. So can we log a warning at least if a Swarm object is finalized but still Running?

Blockchain<BaseAction> chain = _blockchains[0];
Task t = Task.Run(async () =>
{
await swarm.StartAsync(chain, 250);
Copy link
Contributor

Choose a reason for hiding this comment

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

As a separated case, it seems good to test if SwarmException rises when a swarm tries to call StartAsync() twice (i.e., calling StartAsync() when it is already Running).

Blockchain<T> blockchain,
int distributeInterval,
TimeSpan distributeInterval,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@longfin longfin force-pushed the feature/simplify-swarm-interface branch 4 times, most recently from 7447aac to 712f065 Compare February 14, 2019 06:01
@longfin longfin force-pushed the feature/simplify-swarm-interface branch from 712f065 to 09eb0cd Compare February 14, 2019 06:23
@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

Merging #72 into master will decrease coverage by 0.12%.
The diff coverage is 76.54%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage   86.41%   86.28%   -0.13%     
==========================================
  Files          60       60              
  Lines        2672     2684      +12     
==========================================
+ Hits         2309     2316       +7     
- Misses        363      368       +5
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 86.5% <76.54%> (-0.41%) ⬇️
Libplanet/Net/NoSwarmContextException.cs 0% <0%> (-25%) ⬇️
Libplanet/Net/NetMQSocketExtension.cs 68.42% <0%> (+5.26%) ⬆️

@longfin longfin merged commit 92422d8 into planetarium:master Feb 14, 2019
@longfin longfin mentioned this pull request Feb 22, 2019
dahlia pushed a commit to dahlia/libplanet that referenced this pull request Mar 9, 2021
…0-nightly-20191022

Bump libplanet to 0.7.0-nightly.20191022
OnedgeLee pushed a commit to OnedgeLee/libplanet that referenced this pull request Jan 31, 2023
…manifests-1667869217

Update manifests [update-rc-v100321-manifests-1667869217]
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.

2 participants