feat: Added --detached option#33
Conversation
hiikezoe
force-pushed
the
detach
branch
2 times, most recently
from
30cca14 to
8addbd5
Compare
|
@kumar303 r? |
| } else { | ||
| firefox.stdout.destroy(); | ||
| firefox.stdin.destroy(); | ||
| firefox.stderr.destroy(); |
There was a problem hiding this comment.
Though I still think destroying these streams should be done in caller side, to use this 'detached' option in web-ext, we need facebook/flow#5988 in any way, but it's not clear to me when the PR for flow gets merged. So I've changed to destroy the streams here.
There was a problem hiding this comment.
@hiikezoe
I agree with that, and as we briefly discussed over irc, I would really like if we could:
-
be able to keep the child process attached until we are sure that the started Firefox instance is up and running (e.g. in the "web-ext run" use case, we would probably want to wait that we have been able to connect the instance of the chosen remote debugging port) and then
unrefthe child_process from the caller -
be able to re-attach the stdout and stderr streams related to a detached child process (e.g. looking at the related nodejs docs, https://nodejs.org/api/child_process.html#child_process_options_detached, it seems that we could redirect them to a file or a unix socket file and be able to
unrefthe child_process and exit the parent process' event loop without any need to destroy the streams)
How about this additions to the strategy drafted by this PR:
- add two additional parameters to the runFirefox function exported by lib/run.js, e.g.: stdoutFilePath, stderrFilePath (or something similar)
- when the detached options has been specified, set the child_process stdin to 'ignore', and the other two to 'ignore' or to stdoutFilePath and stderrFilePath if they have been specified in the options
how that sounds to you?
(we could also expose these two parameters to the fx-runner bin script, but I'm fine even if we don't expose them from there, I'm not sure that there are so many active users of that bin script)
There was a problem hiding this comment.
That sounds pretty reasonable to me. I will make an update PR soon.
| .then(function(results) { | ||
| var firefox = results.process; | ||
| if (program.verbose) { | ||
| firefox.stdout.pipe(process.stdout) |
There was a problem hiding this comment.
At least with the current implementation, if the 'detached' option has been passed, the firefox.stdout stream has already been destroyed here and so we could probably also skip this firefox.stdout.pipe(process.stdout).
Otherwise, we could destroy the child_process stdio streams at line 41, right before the firefox.unref() call and, when node-fx-runner is used as a library (as in the web-ext CLI tool), the caller is going to decide on its own when it is ready to disconnect the streams and unref the child process.
| } else { | ||
| firefox.stdout.destroy(); | ||
| firefox.stdin.destroy(); | ||
| firefox.stderr.destroy(); |
There was a problem hiding this comment.
@hiikezoe
I agree with that, and as we briefly discussed over irc, I would really like if we could:
-
be able to keep the child process attached until we are sure that the started Firefox instance is up and running (e.g. in the "web-ext run" use case, we would probably want to wait that we have been able to connect the instance of the chosen remote debugging port) and then
unrefthe child_process from the caller -
be able to re-attach the stdout and stderr streams related to a detached child process (e.g. looking at the related nodejs docs, https://nodejs.org/api/child_process.html#child_process_options_detached, it seems that we could redirect them to a file or a unix socket file and be able to
unrefthe child_process and exit the parent process' event loop without any need to destroy the streams)
How about this additions to the strategy drafted by this PR:
- add two additional parameters to the runFirefox function exported by lib/run.js, e.g.: stdoutFilePath, stderrFilePath (or something similar)
- when the detached options has been specified, set the child_process stdin to 'ignore', and the other two to 'ignore' or to stdoutFilePath and stderrFilePath if they have been specified in the options
how that sounds to you?
(we could also expose these two parameters to the fx-runner bin script, but I'm fine even if we don't expose them from there, I'm not sure that there are so many active users of that bin script)
|
Two notes;
|
2 participants
Note that closing pipes process leaves caller side to manage the lifetime on
their own.