Add additional guards for nix process detach #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Observing some weird race-condition-y behaviors where the nix.sh script previously was not reliable in rebooting systems. Most of the time with an attached TTY, the subprocess was still being killed when the TTY-attached parent shell terminated. Adding a `sync` call seems to work around the problem. Adding an explicit `disown` for linux just for completeness. It was not shown to help with the observed race condition (which is resolved by `sync`). `disown` is a bash built-in, and this is a bash script so we should be fine to use it.
reidmv
force-pushed
the
fix-nix-reliability
branch
from
bcc760b
to
9f09672
Compare
MikaelSmith
reviewed
Jun 10, 2019
| @@ -30,7 +30,12 @@ else | |||
| timeout_min=$(($timeout/60)) | |||
| timeout_sec=$(($timeout%60)) | |||
| nohup bash -c "sleep $timeout_sec; shutdown -r +$timeout_min $message" </dev/null >/dev/null 2>&1 & | |||
| disown | |||
There was a problem hiding this comment.
Not sure what scenario would make this necessary, but based on https://unix.stackexchange.com/a/148698 I don't believe it hurts.
MikaelSmith
reviewed
Jun 10, 2019
| # There are some weird race conditions that might make detached jobs still die | ||
| # when this shell exits. The sync (which otherwise shouldn't do much) seems to | ||
| # eliminate them. | ||
| sync |
MikaelSmith
approved these changes
Jun 10, 2019
|
Was it particular Linux variants that had this problem? |
|
I saw the issues on a CentOS 7 VM, on Platform9. Definite race condition-y, sometimes it worked, sometimes it didn't. Worked 100% of the time with the |
|
Thank you for your PR @reidmv :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Observing some weird race-condition-y behaviors where the nix.sh script
previously was not reliable in rebooting systems. Most of the time with
an attached TTY, the subprocess was still being killed when the
TTY-attached parent shell terminated.
Adding a
synccall seems to work around the problem.Adding an explicit
disownfor linux just for completeness. It was notshown to help with the observed race condition, which is resoseems to
work around the problem.
Adding an explicit
disownfor linux just for completeness. It was notshown to help with the observed race condition (which is resolved by
sync).disownis a bash built-in, and this is a bash script so weshould be fine to use it.