-
Notifications
You must be signed in to change notification settings - Fork 236
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
Wait tokio tasks finish before CKB process exit #3999
Wait tokio tasks finish before CKB process exit #3999
Conversation
8c09ab7
to
1cd206f
Compare
2e73c71
to
f69efe1
Compare
f69efe1
to
f22ca81
Compare
6d29416
to
95cc858
Compare
2c7be85
to
4cb2b46
Compare
75514fa
to
4ba0ec3
Compare
Signed-off-by: Eval EXEC <execvy@gmail.com>
Signed-off-by: Eval EXEC <execvy@gmail.com>
Signed-off-by: Eval EXEC <execvy@gmail.com>
Signed-off-by: Eval EXEC <execvy@gmail.com>
4ba0ec3
to
fc9d3e6
Compare
} | ||
|
||
/// Register a thread `JoinHandle` to `CKB_HANDLES` | ||
pub fn register_thread(name: &str, thread_handle: std::thread::JoinHandle<()>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name
is only for printing messages?
so it's not an issue when multiple name
are same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a new version for ckb-stop-handler
?
I'm not sure whether any other projects are using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should.
debug!("wait thread {} done", name); | ||
} | ||
Err(e) => { | ||
warn!("wait thread {}: ERROR: {:?}", name, e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you verify the error log contains panic message?
https://stackoverflow.com/questions/69387239/what-is-in-err-when-i-try-to-join-a-panicked-thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This WARN log didn't print thread's panic message, I should use:
warn!("wait thread {}: ERROR: {:?}",name,e.downcast_ref::<&str>())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update:
If the panic!
use a formatter, e.downcast_ref::<&str>()
will get None
, so I have to use e.downcast_ref::<String>()
:
let msg = e.downcast_ref::<str>().unwrap_or_else(|| {
e.downcast_ref::<String>()
.map(|s| s.as_str())
.unwrap_or("downcast_ref didn't get any error")
});
warn!("wait thread {}: ERROR: {:?}", name, msg)
#!/usr/bin/env bats | ||
bats_load_library 'bats-assert' | ||
bats_load_library 'bats-support' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a test case, some sub-services terminated before receiving exit notification, and then the wait_all_ckb_services_exit
also works for this scenario, maybe also double confirm the log messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can.
I'd like to construct a bats cli test like this:
- start a ckb process.
- append some random bytes to
${DATA_DIR}/data/db/*.sst
- wait the running ckb process'
BlockDownload
thread panic:Lines 171 to 173 in f6466c3
if error_kind == InternalErrorKind::DataCorrupted { panic!("{}", error) } else { - assert that
wait_all_ckb_services_exit
got the panic message.
What problem does this PR solve?
Issue Number: close #3994
Problem Summary:
What is changed and how it works?
What's Changed:
Related changes
Implementation
JoinHandle
CancellationToken
.Test
broadcast_exit_signals
,wait_all_ckb_services_exit
,register_thread
,new_crossbeam_exit_rx
andnew_tokio_exit_rx
Ideas to implement integration test:
ckb run
all ckb threads have been stopped
andckb shutdown
exist in the log file.Check List
Tests
Side effects
Release note