-
Notifications
You must be signed in to change notification settings - Fork 46
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
Optionally treat EOF on stdin just like SIGTERM #111
Merged
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
fea7d78
to
aa38de7
Compare
On Unix, Node.js allows us to gracefully kill a process. On Windows this is more compex. You certainly cannot rely on the default `kill()` function, which calls `TerminateProcess`. There is a bunch of C/C++ extensions that in principle allow you to attempt to gracefully shutdown a Windows process. But, hey, here's a reality check. Node.js controls our stdin. Node.js does IPC easy. Controlling uv_spawn and using the right not well maintained C/C++ Node.js extension to kill a process is fragile. So, treat EOF and any other error on stdin as equivalent to SIGTERM. However, systemd. The sane thing to do with systemd is `StandardInput=null`. With such configuration, stdin immediately returns EOF. Then, introduce the `OONI_STDIN_EOF_IMPLIES_SIGTERM` environment variable. When it is `true`, this behaviour is enabled, e.g.: ```bash export OONI_STDIN_EOF_IMPLIES_SIGTERM=true # behaviour enabled ooniprobe run ``` I want the default to be disabled because: 1. in the future we may find a better way to solve this problem and I don't want the _default behaviour_ to change in such case 2. we know we need this knob for ooniprobe-desktop, and we will not fail to provide it, so it won't suprise/damage us 3. a person trying to write a systemd unit for ooniprobe would be very surprised to find out they need to disable this behaviour, if it was enabled by default by this PR Hence, I believe this design is consistent with designing for the future and for trying to minimize surprises. Also, why an environment variable and not a command line flag? Because: 1. we don't want such hypothetical flag to be available where it does not make sense, e.g., for all subcommands but `run` 2. we don't want the ooni/probe-desktop app to write conditional code because it needs to check the command we're using and then decide whether to add such hypothetical flag Also, why not enabling this only on Windows? Because again we don't want the ooni/probe-desktop app to write conditional code. To summarize: we want ooni/probe-desktop app to see the same behaviour everywhere and we want others to be the least surprised. Related to ooni/probe#1005
aa38de7
to
38e535e
Compare
bassosimone
added a commit
to ooni/probe-desktop
that referenced
this pull request
Feb 13, 2020
See ooni/probe-cli#111 for documentation on the design of killing ooniprobe portably. Closes ooni/probe#1005.
hellais
reviewed
Feb 13, 2020
hellais
approved these changes
Feb 13, 2020
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.
Great work!
Thanks for providing a comprehensive explanation of how it works too.
I left just a very minor nitpick on how to improve the comment, but this looks good to be merged!
bassosimone
added a commit
to ooni/probe-desktop
that referenced
this pull request
Feb 13, 2020
See ooni/probe-cli#111 for documentation on the design of killing ooniprobe portably. Closes ooni/probe#1005.
ainghazal
pushed a commit
to ainghazal/probe-cli
that referenced
this pull request
Mar 8, 2022
* Optionally treat EOF on stdin just like SIGTERM On Unix, Node.js allows us to gracefully kill a process. On Windows this is more compex. You certainly cannot rely on the default `kill()` function, which calls `TerminateProcess`. There is a bunch of C/C++ extensions that in principle allow you to attempt to gracefully shutdown a Windows process. But, hey, here's a reality check. Node.js controls our stdin. Node.js does IPC easy. Controlling uv_spawn flags and using the right not well maintained C/C++ Node.js extension to kill a process is fragile. So, treat EOF and any other error on stdin as equivalent to SIGTERM. However, systemd. The sane thing to do with systemd is `StandardInput=null`. With such configuration, stdin immediately returns EOF. Then, introduce the `OONI_STDIN_EOF_IMPLIES_SIGTERM` environment variable. When it is `true`, this behaviour is enabled, e.g.: ```bash export OONI_STDIN_EOF_IMPLIES_SIGTERM=true # behaviour enabled ooniprobe run ``` I want the default to be disabled because: 1. in the future we may find a better way to solve this problem and I don't want the _default behaviour_ to change in such case 2. we know we need this knob for ooniprobe-desktop, and we will not fail to provide it, so it won't suprise/damage us 3. a person trying to write a systemd unit for ooniprobe would be very surprised to find out they need to disable this behaviour, if it was enabled by default by this PR Hence, I believe this design is consistent with designing for the future and for trying to minimize surprises. Also, why an environment variable and not a command line flag? Because: 1. we don't want such hypothetical flag to be available where it does not make sense, e.g., for all subcommands but `run` 2. we don't want the ooni/probe-desktop app to write conditional code because it needs to check the command we're using and then decide whether to add such hypothetical flag Also, why not enabling this only on Windows? Because again we don't want the ooni/probe-desktop app to write conditional code. To summarize: we want ooni/probe-desktop app to see the same behaviour everywhere and we want others to be the least surprised. Related to ooni/probe#1005 * Update ooni.go
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.
On Unix, Node.js allows us to gracefully kill a process. On Windows
this is more compex. You certainly cannot rely on the default
kill()
function, which calls
TerminateProcess
.There is a bunch of C/C++ extensions that in principle allow you to
attempt to gracefully shutdown a Windows process.
But, hey, here's a reality check. Node.js controls our stdin. Node.js
does IPC easy. Controlling uv_spawn flags and using the right not well maintained
C/C++ Node.js extension to kill a process is fragile.
So, treat EOF and any other error on stdin as equivalent to SIGTERM.
However, systemd.
The sane thing to do with systemd is
StandardInput=null
. With suchconfiguration, stdin immediately returns EOF.
Then, introduce the
OONI_STDIN_EOF_IMPLIES_SIGTERM
environmentvariable. When it is
true
, this behaviour is enabled, e.g.:I want the default to be disabled because:
in the future we may find a better way to solve this problem and I
don't want the default behaviour to change in such case
we know we need this knob for ooniprobe-desktop, and we will not
fail to provide it, so it won't suprise/damage us
a person trying to write a systemd unit for ooniprobe would be very
surprised to find out they need to disable this behaviour, if it was
enabled by default by this PR
Hence, I believe this design is consistent with designing for the
future and for trying to minimize surprises.
Also, why an environment variable and not a command line flag? Because:
we don't want such hypothetical flag to be available where it does not
make sense, e.g., for all subcommands but
run
we don't want the ooni/probe-desktop app to write conditional
code because it needs to check the command we're using and then decide
whether to add such hypothetical flag
Also, why not enabling this only on Windows? Because again we don't
want the ooni/probe-desktop app to write conditional code.
To summarize: we want ooni/probe-desktop app to see the same behaviour
everywhere and we want others to be the least surprised.
Related to ooni/probe#1005