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.

Sign up for GitHub

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

Jump to bottom

Make the node open its ports before doing discovery() #1118

Merged
merged 2 commits into from
Aug 14, 2020
Merged

Make the node open its ports before doing discovery() #1118

merged 2 commits into from
Aug 14, 2020

Conversation

AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Aug 13, 2020 •
edited
Loading

Another small part towards fixing #1117.

@AndrejMitrovic AndrejMitrovic added the type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense label Aug 13, 2020
@Geod24
Copy link
Collaborator

Geod24 commented Aug 13, 2020 •
edited
Loading

We really need that schedule primitive...

@linked0
Copy link
Contributor

linked0 commented Aug 14, 2020 •
edited
Loading

All the calls in the node.start() were also executed asynchronously before through the Vibe.d's timer and asynchronous tasks. Is there a big difference between before and after? The commit message says that this makes the node not to wait for the first discovery round to complete though.

@AndrejMitrovic
Copy link
Contributor Author

I'll rename the commit and change the description.

@AndrejMitrovic AndrejMitrovic changed the title Make the node start call asynchronous Make the node start call non-blocking Aug 14, 2020
@codecov
Copy link

codecov bot commented Aug 14, 2020 •
edited
Loading

Codecov Report

Merging #1118 into v0.x.x will increase coverage by 1.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #1118      +/-   ##
==========================================
+ Coverage   77.11%   78.56%   +1.45%     
==========================================
  Files          83       84       +1     
  Lines        7961     7978      +17     
==========================================
+ Hits         6139     6268     +129     
+ Misses       1822     1710     -112
Flag Coverage Δ
#integration 39.75% <100.00%> (?)
#unittests 77.11% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/node/Runner.d 65.11% <100.00%> (+65.11%) ⬆️
source/agora/node/main.d 56.25% <0.00%> (ø)
source/agora/node/BlockStorage.d 72.37% <0.00%> (+0.69%) ⬆️
source/agora/node/Validator.d 94.73% <0.00%> (+1.75%) ⬆️
source/agora/consensus/protocol/Nominator.d 89.58% <0.00%> (+2.60%) ⬆️
source/agora/common/Config.d 91.39% <0.00%> (+3.31%) ⬆️
source/agora/network/NetworkManager.d 76.16% <0.00%> (+6.21%) ⬆️
source/agora/node/FullNode.d 83.33% <0.00%> (+8.68%) ⬆️
source/scpd/Cpp.d 94.73% <0.00%> (+14.73%) ⬆️
source/agora/network/NetworkClient.d 89.36% <0.00%> (+19.14%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 331e518...28b4b06. Read the comment docs.

@AndrejMitrovic
Make the node call listenHTTP first
41d0d48 
The runTask() call will spawn a new fiber and
and switch to the new fiber immediately.
However we want to start listening for incoming
requests before doing network discovery.

Using setTimer with a zero timeout (a.k.a. schedule())
makes the node open a listening port right away,
rather than waiting for the first discovery round
to complete.

It increases the node's connectivity to other nodes
which may want to establish a connection,
and consequently reduces CI failures.

Part of: bosagora#1117
@AndrejMitrovic
Try to reduce request retries & timeouts
28b4b06
@AndrejMitrovic AndrejMitrovic changed the title Make the node start call non-blocking Make the node open its ports before doing discovery() Aug 14, 2020
@AndrejMitrovic
Copy link
Contributor Author

The issue wasn't really about asynchronicity, it was about scheduling.

The runTask call would spawn a new fiber and suspend the calling fiber, and the new fiber would spawn many other requests. This starved the first fiber (the one in main.d calling runNode) - so it only gets to call listenHTTP later.

But we want listenHTTP to be called right away to make the node available for incoming connections.

Fixed the description.

@linked0
Copy link
Contributor

linked0 commented Aug 14, 2020 •
edited
Loading

With your explanation, I've just realized that this is about scheduling and just seen the log shown in #1117. Thanks for the detailed explanation.

@AndrejMitrovic
Copy link
Contributor Author

Thanks. I'll merge it with the approval.

@AndrejMitrovic AndrejMitrovic merged commit 2f4efaa into bosagora:v0.x.x Aug 14, 2020
@AndrejMitrovic AndrejMitrovic deleted the async-start branch August 14, 2020 03:01
@AndrejMitrovic AndrejMitrovic mentioned this pull request Aug 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linked0 linked0 linked0 approved these changes

Assignees
No one assigned
Labels
type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndrejMitrovic @Geod24 @linked0