TOOLS-2366: Update signal handler to exit after first signal #41
Conversation
sphanse99
requested review from
huan-Mongo,
mattChiaravalloti,
tfogo and
varsha804
|
Oh! Actually one follow up at the commit message. Have you seen this resource: https://chris.beams.io/posts/git-commit/ ? Can you update this PR's commit message (the title) to follow those guidelines? Maybe something like "TOOLS-2366: Update signal handler to exit after first signal" or something |
sphanse99
changed the title
| // first signal use finalizer to terminate cleanly | ||
| switch sig := <-sigChan; sig { | ||
| // SIGINT & SIGTERM: use finalizer & terminate cleanly | ||
| case syscall.SIGINT, syscall.SIGTERM: |
There was a problem hiding this comment.
A question is for syscall.SIGINT, syscall.SIGTERM, do we need to follow the previous design, i.e. call finalizer to clean up the resources such as doc channel etc on the first signal and then exit on the second signal. With current change, I believe restore will exit with the first ^C attempt.
There was a problem hiding this comment.
Good question, finalizer() is still called in this implementation (line 47), but now we call os.Exit() after the finalizer instead of waiting for the second signal. We decided this design was more intuitive. Let me know if further clarification would be helpful!
Although, I wonder if we should send a os.Exit(util.ExitSuccess) here since we did shutdown processes through finalizer... thoughts? Not entirely sure what the difference between util.ExitFailure and util.ExitSuccess is.
There was a problem hiding this comment.
Cool. It's OK then if you are aware this change from past design.
Btw, I don't think we shutdown process in the finalizer, looking from restore.HandleInterrupt, it's just close channels instead of callingos.Exit. So we need to call the exit here.
The difference between util.ExitFailure and util.ExitSuccess is the status code program exits.
There was a problem hiding this comment.
Good correction. In that case, do you think it's more appropriate to use ExitSuccess or ExitFailure here (or does it matter)?
There was a problem hiding this comment.
Since in the previous code it uses ExitFailure, I think it's fine to use ExitFailure here.
There was a problem hiding this comment.
Huan is correct that the existing (ignore this, I agree with the outcome you guys came up with)finalizer functions don't call os.Exit (though it is possible for a caller of this function to do that if they want). I think os.Exit(util.ExitSuccess) makes sense when we shutdown gracefully. Not sure what the convention for this is.
There was a problem hiding this comment.
Um, after a second thought, I believe there is some other content. So in the previous design, if you hit ^C once, it will call finalizer to shutdown channel and after a short time, the main function should exit gracefully, which would return ExitSuccess. Now, since you changed the two step procedure, the program will exit in the first attempt, then the program might not shutdown gracefully. We should return a ExitFailure here.
So the extra content is in the previous design, if you hit ^C once, the program should not immediately, but eventually shutdown gracefully. But now it would immediate exit with a failure status code.
There was a problem hiding this comment.
and after a short time, the main function should exit gracefully
Yeah, it seems like that is currently happening correctly. I think I originally saw mongodump hanging instead of exiting, but I can't reproduce that right now. @sphanse99, did you observe the hanging behavior in your testing?
Re exit codes, I think exiting with 0 or 1 when catching a signal is incorrect behavior. The POSIX spec says the exit code should be greater than 128 when a program is terminated due to a signal. It seems that most bash-like shells construct this as 128 + n where n is the code of the signal used to terminate the program. So a SIGINT should exit with a code 130 for example. However, it looks like this varies between shells. I read a recommendation to do the following to ensure the proper exit code:
- Catch the signal and do any necessary cleanup.
- Stop catching the signal.
- Send a the signal to the current process and let the OS terminate it up and generate the correct exit code.
The recommendation says there is no way to fake the correct exit code, so sending a signal to yourself is the only way to ensure correct behavior.
There was a problem hiding this comment.
I observed the hanging when I started the ticket, but also noticed this morning that I couldn't reproduce it. It is unclear why this behavior changed -- I will investigate further!
No description provided.