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

Ensure Single Context Used for Beacon Node Cancelation on SIGTERM #5671

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Apr 29, 2020

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

We were confusing the usage of the cli context with a regular context used by the beacon node in many different services. So when we would attempt to stop the beacon node process while it was doing some expensive operation, such as regenerating historical states, the node would not stop as it would not be able to understand the root context had been canceled. This PR simplifies the code and ensures we have a global, single ctx for the beacon node in node/node.go which is propagated down to all running services.

This PR also adds a check for ctx.Err() on each iteration of regenerating historical states, which is an expensive operation.

Which issues(s) does this PR fix?

Fixes #5470

@rauljordan rauljordan requested a review from a team as a code owner April 29, 2020 02:06
@rauljordan rauljordan self-assigned this Apr 29, 2020
@rauljordan rauljordan added OK to merge Ready For Review A pull request ready for code review labels Apr 29, 2020
@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #5671 into master will increase coverage by 27.12%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           master    #5671       +/-   ##
===========================================
+ Coverage   20.04%   47.17%   +27.12%     
===========================================
  Files         240      239        -1     
  Lines       20940    20839      -101     
===========================================
+ Hits         4198     9831     +5633     
+ Misses      15915     9217     -6698     
- Partials      827     1791      +964     

@prylabs-bulldozer prylabs-bulldozer bot merged commit 9415519 into master Apr 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the proper-ctx branch April 29, 2020 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot kill beacon node when regenerating states
2 participants