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

Security: Stop RocksDB or tokio calling unexpected code when zebrad exits #3392

Merged
merged 12 commits into from
Jan 26, 2022
Merged

Security: Stop RocksDB or tokio calling unexpected code when zebrad exits #3392

merged 12 commits into from
Jan 26, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jan 24, 2022 •
edited
Loading

Motivation

Fix warnings coming from C++(most probably rocksdb) that are displayed sometimes when Ctrl-C is executed to exit zebrad.

Solution

Bug Fix:

  • Wait for RocksDB background tasks to finish before exiting
  • Use the tokio runtime to shutdown (remove quick fix introduced in Re-order Zebra startup, so slow services are launched last #3091)
  • Add a timeout to tokio runtime of 20 seconds max to shutdown
  • Shut down the same way during normal operation and debug_stop_at_height tests
  • Log when shutting down

Testing:

  • Add a shell script that checks Zebra for shutdown errors

Closes #3133.

Review

@teor2345 and i both worked on it but anyone can review.

@oxarbitrage oxarbitrage mentioned this pull request Jan 24, 2022
Closed
@codecov
Copy link

codecov bot commented Jan 24, 2022 •
edited
Loading

Codecov Report

Merging #3392 (1952872) into main (499ae89) will increase coverage by 0.08%.
The diff coverage is 85.45%.

@@            Coverage Diff             @@
##             main    #3392      +/-   ##
==========================================
+ Coverage   78.34%   78.42%   +0.08%     
==========================================
  Files         267      267              
  Lines       31526    31576      +50     
==========================================
+ Hits        24698    24764      +66     
+ Misses       6828     6812      -16

@teor2345 teor2345 changed the title Update shutdown Security: Stop RocksDB or tokio calling unexpected code when zebrad exits Jan 25, 2022
@teor2345 teor2345 self-requested a review January 25, 2022 00:49
teor2345
teor2345 reviewed Jan 25, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's see if waiting for the database in cancel_all_background_work fixes the errors we're seeing.

I also added some optional changes, we should only do them if this PR actually fixes the errors.

@teor2345
Copy link
Contributor

teor2345 commented Jan 25, 2022 •
edited
Loading

I opened oxarbitrage#164 with some fixes and a test script. It's based on this PR.

I would be happy to merge oxarbitrage#164 or whatever you think is best here.

I ran a bunch of tests using the new shutdown-errors test script. They failed on main, but worked on this PR and on oxarbitrage#164.

@teor2345 teor2345 mentioned this pull request Jan 25, 2022
Merged
3 tasks
@oxarbitrage oxarbitrage marked this pull request as ready for review January 25, 2022 17:08
@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2022

update

✅ Branch has been successfully updated

teor2345
teor2345 approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, let's merge!

@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2022

refresh

✅ Pull request refreshed

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2022

update

☑️ Nothing to do

  • -closed [:pushpin: update requirement]
  • #commits-behind>0 [:pushpin: update requirement]

@mergify mergify bot merged commit cc594f1 into ZcashFoundation:main Jan 26, 2022
@teor2345 teor2345 mentioned this pull request Feb 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

Assignees

@oxarbitrage oxarbitrage

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Security: Stop RocksDB threads calling unexpected code when zebrad exits
3 participants
@oxarbitrage @teor2345 @gustavovalverde