Fix network discovery & start Node asynchronously #648
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrejMitrovic
added
type-bug
Codecov Report
@@ Coverage Diff @@
## v0.x.x #648 +/- ##
==========================================
+ Coverage 90.13% 90.21% +0.07%
==========================================
Files 62 62
Lines 4623 4648 +25
==========================================
+ Hits 4167 4193 +26
+ Misses 456 455 -1
Continue to review full report at Codecov.
|
|
The asynchronous start has managed to find a bug: https://github.com/bpfkorea/agora/pull/649/files |
AndrejMitrovic
force-pushed
the
fix-discovery
branch
2 times, most recently
from
d2041b1
to
1683f81
Compare
AndrejMitrovic
added a commit
to AndrejMitrovic/agora
that referenced
this pull request
Mar 11, 2020
It will be used in a later test.
AndrejMitrovic
force-pushed
the
fix-discovery
branch
2 times, most recently
from
f294696
to
c8831a1
Compare
Otherwise using 'dtest=agora.test.Network dub test' will also run tests in agora.test.NetworkClient.d and agora.test.NetworkManager.d, because both partially match the string.
This allows the test-suite to start multiple nodes in parallel without waiting for the discovery phase of one node to complete before another node's discovery begins. Without this we cannot test network discovery properly due to a race condition: A.start() => calls B.getNetworkInfo() However B's network info will always be empty because B.start() was never called, because the main thread is stuck waiting for A.start() to return. Fixes bosagora#276
The network info was discarded instead of used. Fixes bosagora#647
AndrejMitrovic
force-pushed
the
fix-discovery
branch
from
c8831a1
to
7e69a41
Compare
|
@bpfkorea/core this is blocking other PRs. Could you please review? :) |
linked0
approved these changes
Mar 12, 2020
TrustHenry
approved these changes
Mar 12, 2020
Geod24
reviewed
Mar 12, 2020
| this.network.discover(); | ||
|
|
||
| bool isNominating () | ||
| this.taskman.runTask( |
There was a problem hiding this comment.
There was a problem hiding this comment.
I saw that, but I think I left it in out of misunderstanding. I'll remove it in a fixup PR.
There was a problem hiding this comment.
Oh, I didn't see this in detail.
It was wrapped twice.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the network discovery issue #647. While creating a test-case I've realized that #276 is actually blocking the implementation of a test-case. So this PR includes two fixes.
Fixes #276
Fixes #647