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
Revert "refactor(exec): remove paramsFile (#807)" #819
Conversation
This reverts commit 64d5899. Reason for revert: If stdin is large, then the param object can become an extremely long string, exceeding the maximum OS size limit on commandline parameters. Original change's description: > refactor(exec): remove paramsFile (#807) > > The `paramsFile` is obsolete now that we use `execFileSync()` for our > internal implementation. Instead, we pass parameters to the child > process directly as a single commandline parameter to reduce file I/O. > > Issue #782 Fixes #818
Tested by creating a 6M lorem ipsum file: shell.cat('lorem.txt').exec('tr o a').to('out.txt');
// crashes without this revert |
@@ -21,6 +21,7 @@ function execSync(cmd, opts, pipe) { | |||
} | |||
|
|||
var tempDir = _tempDir(); | |||
var paramsFile = path.resolve(tempDir + '/' + common.randomFileName()); |
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 doesn't need to be fixed now, but I think the paths should be joined with path.join
, instead of just concatenating the strings... future improvements.
Travis mac builders appear to be stuck. I'm just going to land this, because I'm pretty confident it's safe. |
This reverts commit 64d5899.
Reason for revert: If stdin is large, then the param object can become
an extremely long string, exceeding the maximum OS size limit on
commandline parameters.
Original change's description:
Fixes #818