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 |
| } | ||
|
|
||
| c.stdout.on('end', stdoutStream.end); | ||
| c.stderr.on('end', stderrStream.end); |
There was a problem hiding this comment.
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.
Seems reasonable. Do you think it's worth adding this to similar files (e.g. bin/shjs)?
There was a problem hiding this comment.
Yes, I think this would be a good idea for other executable-only files.
There was a problem hiding this comment.
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.
Failing loudly sounds like a good idea.
| codeFile: codeFile, | ||
| }; | ||
|
|
||
| fs.writeFileSync(paramsFile, JSON.stringify(paramsToSerialize), 'utf8'); |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
I think there's an eslint warning against that...
|
@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