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

Issue 2772 - part A: Refactor timers in Nominator #3024

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Issue 2772 - part A: Refactor timers in Nominator #3024

merged 1 commit into from
Feb 14, 2022

Conversation

hewison-chris
Copy link
Contributor

@hewison-chris hewison-chris commented Feb 10, 2022 •
edited by linked0
Loading

Using a similar approach to timers in the FullNode and Validator classes.
This is partly in preparation for adding another timer for checking when we can externalize with majority of signed
in PR #3026

Part of #2772

@codecov
Copy link

codecov bot commented Feb 10, 2022 •
edited
Loading

Codecov Report

Merging #3024 (8f8d58b) into v0.x.x (83349fd) will increase coverage by 0.45%.
The diff coverage is n/a.

❗ Current head 8f8d58b differs from pull request most recent head b330e63. Consider uploading reports for the commit b330e63 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #3024      +/-   ##
==========================================
+ Coverage   19.83%   20.28%   +0.45%     
==========================================
  Files          61       68       +7     
  Lines        2702     2923     +221     
==========================================
+ Hits          536      593      +57     
- Misses       2166     2330     +164
Flag Coverage Δ
integration 20.28% <ø> (+0.45%) ⬆️

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

Impacted Files Coverage Δ
source/agora/stats/Stats.d 52.94% <0.00%> (ø)
source/agora/stats/Utils.d 100.00% <0.00%> (ø)
source/agora/script/Stack.d 0.00% <0.00%> (ø)
source/agora/utils/Backoff.d 0.00% <0.00%> (ø)
source/agora/stats/Server.d 0.00% <0.00%> (ø)
source/agora/utils/InetUtils.d 0.00% <0.00%> (ø)
source/agora/utils/Log.d 40.90% <0.00%> (ø)

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 edf2052...b330e63. Read the comment docs.

linked0
linked0 reviewed Feb 10, 2022
View reviewed changes
source/agora/consensus/protocol/Nominator.d Outdated
Comment on lines 627 to 631
// ignore messages if `startNominatingTimer` was never called or
// if `stopNominatingTimer` was called
if (this.nomination_timer is null)
return;

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this removal be in another commit or the second commit because it changes the handling algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't work as expected so is reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strangely ValidatorCleanRestart passes without removing this 🤔

@hewison-chris hewison-chris force-pushed the refactor_timers_nomination branch from 8f8d58b to 1cf5e28 Compare February 10, 2022 08:11
@hewison-chris
Copy link
Contributor Author

🟢

linked0
linked0 approved these changes Feb 10, 2022
View reviewed changes
Copy link
Contributor

@linked0 linked0 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, Do you think this PR is also meaningful to solve the failure of ValidatorCleanRestart? I just wanted to know that this is related to issue #2971.

@hewison-chris
Copy link
Contributor Author

LGTM, Do you think this PR is also meaningful to solve the failure of ValidatorCleanRestart? I just wanted to know that this is related to issue #2971.

I am not sure. I will create a separate PR with just the ValidatorCleanRestart against v0.x.x branch to see if something else already fixed it.

@hewison-chris
Copy link
Contributor Author

LGTM, Do you think this PR is also meaningful to solve the failure of ValidatorCleanRestart? I just wanted to know that this is related to issue #2971.

I am not sure. I will create a separate PR with just the ValidatorCleanRestart against v0.x.x branch to see if something else already fixed it.
It did pass in PR #3031 so I will remove from this PR.

@hewison-chris hewison-chris force-pushed the refactor_timers_nomination branch 2 times, most recently from 18a3f2a to 6da9970 Compare February 11, 2022 05:55
@hewison-chris hewison-chris self-assigned this Feb 11, 2022
@hewison-chris hewison-chris changed the title Refactor timers in Nominator Issue 2772 - A: Refactor timers in Nominator Feb 11, 2022
@hewison-chris hewison-chris changed the title Issue 2772 - A: Refactor timers in Nominator Issue 2772 - part A: Refactor timers in Nominator Feb 11, 2022
This was referenced Feb 11, 2022
Closed
Merged
@hewison-chris hewison-chris force-pushed the refactor_timers_nomination branch from 6da9970 to 87f499d Compare February 11, 2022 07:34
omerfirmak
omerfirmak approved these changes Feb 14, 2022
View reviewed changes
source/agora/consensus/protocol/Nominator.d
this.nomination_timer.stop();
this.nomination_timer = null;
this.timers[TimersIdx.Nomination].stop();
this.timers[TimersIdx.Nomination] = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually dont need to null it here, we can reuse the same timer every slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try to remove it but we compare it to null here:

agora/source/agora/consensus/protocol/Nominator.d

Lines 625 to 630 in 83349fd

private void handleSCPEnvelope (in SCPEnvelope envelope) @trusted
{
// ignore messages if `startNominatingTimer` was never called or
// if `stopNominatingTimer` was called
if (this.nomination_timer is null)
return;

I don't think we can check if the timer is stopped.
Maybe pending could be used but I think it may be false when actually running the task.

Copy link
Contributor

Choose a reason for hiding this comment

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

then feel free to merge when CI is green

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, rebased on a green upstream 🤞

@hewison-chris
Refactor timers used in Nominator
b330e63 
Use enums to identify timers within an array of timers. Just create them
in the constructor and arm them when needed. Stop all timers on
shutdown.
@hewison-chris hewison-chris force-pushed the refactor_timers_nomination branch from 87f499d to b330e63 Compare February 14, 2022 07:57
@hewison-chris
Copy link
Contributor Author

Will merge this as this commit is the first in #3037 which is green.

@hewison-chris hewison-chris merged commit 8cc309e into bosagora:v0.x.x Feb 14, 2022
@hewison-chris hewison-chris linked an issue Feb 22, 2022 that may be closed by this pull request
Race condition on shutdown can cause SEGV #2942
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omerfirmak omerfirmak omerfirmak approved these changes

@linked0 linked0 linked0 approved these changes

Assignees

@hewison-chris hewison-chris

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race condition on shutdown can cause SEGV
3 participants
@hewison-chris @linked0 @omerfirmak