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

ConnectionTask: Fix request failure retrying #1106

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

AndrejMitrovic
Copy link
Contributor

This might resolve the frequent CircleCI failures we've seen recently.

Fixes #1105

Previously if the request failed after X number of
attempts this would throw an Exception which would
not be caught in the ConnectionTask.

Instead the request should be continuously retried
until the request was successful or until the
BanManager bans the node.

Fixes bosagora#1105
@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 7, 2020
The per-request timeout is too high. The waitForDiscovery()
call in the integration test only waited 5 seconds,
but this doesn't give us enough time to attempt a request again.
@AndrejMitrovic
Copy link
Contributor Author

The CI still failed so I'm trying another thing in a second commit.

@AndrejMitrovic
Copy link
Contributor Author

The CI failed

But the "uncaught exception" errors are now gone so this is good.

@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

Merging #1106 into v0.x.x will increase coverage by 1.25%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #1106      +/-   ##
==========================================
+ Coverage   77.30%   78.55%   +1.25%     
==========================================
  Files          81       83       +2     
  Lines        7886     7912      +26     
==========================================
+ Hits         6096     6215     +119     
+ Misses       1790     1697      -93     
Flag Coverage Δ
#integration 39.56% <33.33%> (?)
#unittests 77.35% <72.72%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
source/agora/network/NetworkManager.d 75.39% <33.33%> (+6.44%) ⬆️
source/agora/test/Timeout.d 87.50% <87.50%> (ø)
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/common/Config.d 91.39% <0.00%> (+3.31%) ⬆️
source/agora/node/FullNode.d 84.72% <0.00%> (+8.66%) ⬆️
source/scpd/Cpp.d 94.73% <0.00%> (+14.73%) ⬆️
... 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 ef0b8e7...430d1d7. Read the comment docs.

@Geod24 Geod24 merged commit 5e25f0e into bosagora:v0.x.x Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

ConnectionTask is not handling request timeouts gracefully
2 participants