-
Notifications
You must be signed in to change notification settings - Fork 733
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
refactor(exec): move child process to source file #786
Conversation
This PR refactors `shell.exec()` by putting its child process in a separate code file. This also slightly cleans up dead code. There's more potential to clean this up (e.g. exit status), but this is a good enough start. Issue #782
@freitagbr I will also land separate CLs (against the same issue) for:
|
Codecov Report
@@ Coverage Diff @@
## master #786 +/- ##
========================================
+ Coverage 95.39% 95.8% +0.4%
========================================
Files 33 34 +1
Lines 1239 1360 +121
========================================
+ Hits 1182 1303 +121
Misses 57 57
Continue to review full report at Codecov.
|
@freitagbr ping |
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.
Just a few small ideas, otherwise this looks promising.
} | ||
|
||
c.stdout.on('end', stdoutStream.end); | ||
c.stderr.on('end', stderrStream.end); |
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.
If this file is require
'd, it will actually do something. To prevent this, wrap the contents like this:
if (require.main === module) {
// everything goes here
}
This way, the code is only executed when the file is the entrypoint, like when node exec-file.js
is run.
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.
Seems reasonable. Do you think it's worth adding this to similar files (e.g. bin/shjs
)?
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.
Yes, I think this would be a good idea for other executable-only files.
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.
Filed #789. I think it would be better to fail loudly in this case (with an exception). Take a look at my alternate suggestion, let me know if there's some issue with this.
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.
Failing loudly sounds like a good idea.
codeFile: codeFile, | ||
}; | ||
|
||
fs.writeFileSync(paramsFile, JSON.stringify(paramsToSerialize), 'utf8'); |
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.
What is the advantage of storing the params in a separate file, as opposed to simply passing them in as a argument to the command?
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.
That was my original approach, but there's some issues with passing via CLI argument (at least in shells, via execSync
). As an example, we could look at the multi-line string:
has"chars
.txt
JSON.stringify
gives us "has\"chars\n.txt"
. So far, so good.
When this gets passed to the shell, it looks something like:
node exec-child.js "has\"chars\n.txt"
The shell interprets the string and supplies the interpreted value to the node process, so process.argv[2]
looks like has"chars\n.txt
(that's a one-line string with a literal \
followed by a literal n
).
This is where we hit issues. Our string is partially deserialized, so we can't use JSON.parse
, but it's still not equivalent to our original string. This is an unfortunate consequence of using both shell and JS--they have different opinions on how to escape strings.
We can consider passing these by CLI argument if we replace execSync()
with execFileSync()
within exec.js
. This is one of my intended future optimizations, but I left it out to limit complexity. Such a change is backwards compatible (we just can't change exec()
within exec-child.js
).
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.
Ok, that makes sense. We eliminate the need to write the JS file every call, but in return we need to write the arguments to a file. Not the best, but it's an improvement overall.
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.
This is still strictly better. The JS file used to contain each argument written inside it (as literals), and sizeof(code) + sizeof(arguments) > sizeof(arguments)
.
I'll also send some PRs to trim down the number of arguments, which will be a bigger win.
try { common.unlinkSync(codeFile); } catch (e2) {} | ||
try { common.unlinkSync(paramsFile); } catch (e2) {} | ||
try { common.unlinkSync(stderrFile); } catch (e2) {} | ||
try { common.unlinkSync(stdoutFile); } catch (e2) {} |
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.
We're not using e2
anyway, might as well call it e
(it won't overwrite the e
from above).
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.
I think there's an eslint warning against that...
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.
Oh, right.
@freitagbr take a look at my comments. I'll upload a change tonight for your first comment (I don't think there's any work needed for the other two). |
PTAL @freitagbr |
LGTM |
This PR refactors
shell.exec()
by putting its child process in a separate codefile. This also slightly cleans up dead code.
There's more potential to clean this up (e.g. exit status), but this is a good
enough start.
Issue #782