Signal shutdown #1599
Signal shutdown #1599
Conversation
maackle
requested review from
lucksus,
struktured,
StaticallyTypedAnxiety,
Connoropolous and
willemolding
| let refs = Arc::strong_count(&CONDUCTOR); | ||
| if refs == 1 { | ||
| println!("Gracefully shutting down conductor..."); | ||
| } else { |
There was a problem hiding this comment.
what's the difference between 'graceful' shutdown and 'explicit' shutdown?
(context, I've lost the ability to fully track/understand some of the more sophisticated Rust code that's being written by y'all now)
There was a problem hiding this comment.
I guess the idea is that if the reference count for the CONDUCTOR Arc is 1 there is nobody else (no other thread) alive and running for this conductor and we don't need to explicitly call conductor.shutdown() as that is called in the Drop implementation.
But I don't completely get the difference either since we take ownership of the conductor in line 75 with mem::replace, right? So that object will be dropped at the end of the for-body's scope either way.. Isn't that right, @maackle?
There was a problem hiding this comment.
Yes, this bit is a little confusing, but I figured it's better to have something than nothing. I was thinking that other threads would still have access to a conductor in the explicit shutdown case, but @lucksus is right that the conductor gets dropped either way. However, if there are other threads referencing CONDUCTOR at this point, they would wind up with a None when they expected Some(Conductor), so this warning at least explains why. I could modify the warning message a little, but I think it still mostly makes sense like this.
|
@maackle could you toss a changelog item on here? |
| for _sig in termination_signals.forever() { | ||
| let mut conductor_guard = CONDUCTOR.lock().unwrap(); | ||
| let conductor = std::mem::replace(&mut *conductor_guard, None); | ||
| let refs = Arc::strong_count(&CONDUCTOR); |
There was a problem hiding this comment.
do we care about dropping the value in the conductor_guard?
There was a problem hiding this comment.
@AshantiMutinta it will get dropped at the end of the block. What's your concern?
conductor_api/src/conductor/base.rs
Outdated
| .stop_all_instances() | ||
| .map_err(|error| notify(format!("Error during shutdown: {}", error))); | ||
| pub fn shutdown(&mut self) -> Result<(), HolochainInstanceError> { | ||
| let _ = self.stop_all_instances()?; |
There was a problem hiding this comment.
can this just be self.stop_all_instances()?
Co-Authored-By: Willem Olding <willemolding@gmail.com>
There was a problem hiding this comment.
Hm, I guess this brings us one step away from native Windows support: "It should work on any POSIX.1-2001 system, which are all the major big OSes with the notable exception of Windows."
Should we find a solution for windows as well or is this a real problem only in our app-spec CI?
| let refs = Arc::strong_count(&CONDUCTOR); | ||
| if refs == 1 { | ||
| println!("Gracefully shutting down conductor..."); | ||
| } else { |
There was a problem hiding this comment.
I guess the idea is that if the reference count for the CONDUCTOR Arc is 1 there is nobody else (no other thread) alive and running for this conductor and we don't need to explicitly call conductor.shutdown() as that is called in the Drop implementation.
But I don't completely get the difference either since we take ownership of the conductor in line 75 with mem::replace, right? So that object will be dropped at the end of the for-body's scope either way.. Isn't that right, @maackle?
PR summary
Allows conductor to shut down gracefully upon SIGINT (Ctrl+C) or SIGKILL, which also allows n3h to shut down gracefully
Necessary for try-o-rama to kill and respawn a conductor during a test
testing/benchmarking notes
There is no test written for this. It will have to be tested manually, and later, implicitly through try-o-rama tests
changelog
Please check one of the following, relating to the CHANGELOG-UNRELEASED.md
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)