Skip to content

Upgrade checking handlers for Stoa#3165

Open
linked0 wants to merge 1 commit intobosagora:v0.x.xfrom
linked0:2942-check-handler
Open

Upgrade checking handlers for Stoa#3165
linked0 wants to merge 1 commit intobosagora:v0.x.xfrom
linked0:2942-check-handler

Conversation

@linked0
Copy link
Contributor

@linked0 linked0 commented Mar 4, 2022

After shutting down nodes, there are cases where nodes crash
because the handlers for Stoa are nullified. So this is a way
to keep from crashing by using the handler structure itself, not
the index.

Fixes #2942

@linked0 linked0 force-pushed the 2942-check-handler branch from 75bfb96 to 334a094 Compare March 4, 2022 06:56
@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #3165 (a33b4a9) into v0.x.x (c60c869) will increase coverage by 0.21%.
The diff coverage is 20.83%.

@@            Coverage Diff             @@
##           v0.x.x    #3165      +/-   ##
==========================================
+ Coverage   88.15%   88.37%   +0.21%     
==========================================
  Files         165      165              
  Lines       17067    17079      +12     
==========================================
+ Hits        15045    15093      +48     
+ Misses       2022     1986      -36     
Flag Coverage Δ
integration 39.37% <0.00%> (+2.57%) ⬆️
unittests 87.74% <20.83%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
source/agora/node/FullNode.d 73.41% <20.83%> (+0.96%) ⬆️
source/agora/consensus/BlockStorage.d 75.61% <0.00%> (-1.77%) ⬇️
source/agora/consensus/protocol/Nominator.d 90.94% <0.00%> (-0.54%) ⬇️
source/agora/script/Engine.d 97.34% <0.00%> (-0.15%) ⬇️
source/agora/test/Base.d 80.70% <0.00%> (+0.22%) ⬆️
source/agora/network/Manager.d 77.21% <0.00%> (+0.31%) ⬆️
source/agora/consensus/state/Ledger.d 89.50% <0.00%> (+0.65%) ⬆️
source/agora/node/Validator.d 92.46% <0.00%> (+1.00%) ⬆️
source/agora/node/Config.d 68.61% <0.00%> (+3.64%) ⬆️
... and 2 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 c60c869...a33b4a9. Read the comment docs.

@linked0
Copy link
Contributor Author

linked0 commented Mar 4, 2022

Ready for review. The spurious error on CI happens tho.

@Geod24
Copy link
Contributor

Geod24 commented Mar 4, 2022

This is not fixing the issue IMO, for the same reason that previous changes didn't fix the issue.
After Ctrl+C is called and the shutdown is initiated, nothing should be called, because we cannot sanely shut down in the signal handler if things are going to fire randomly afterwards. However, we're seeing via this segv that things DO get called. Additionally, #3158 seems to suggest that this trigger in prod, too.

After shutting down nodes, there are cases where nodes crash
because the handlers for Stoa are nullified. So this is a way
to keep from crashing by using a handler structure itself not
the index`.
@linked0 linked0 force-pushed the 2942-check-handler branch from 334a094 to a33b4a9 Compare April 15, 2022 08:18
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.

Race condition on shutdown can cause SEGV

2 participants