Remove codeFile parameter #791
Conversation
This parameter isn't needed, we can easily rely on exit code status for this. Eliminating the parameter reduces file IO, code complexity, and removes a busy loop. Issue #782
src/exec-child.js
Outdated
| }); | ||
| } catch (e) { | ||
| // child_process could not run the command. | ||
| process.exit(127); |
There was a problem hiding this comment.
process.exit is potentially a bad idea if there are still callbacks in the queue, or if there are async tasks running, because they will not finish. Of course, at this point, neither of these should be the case, but I still think setting process.exitCode is a safer way to do this.
There was a problem hiding this comment.
Also, below this, we are manually ending stdoutStream and stderrStream when c.stdout and c.stderr end. Why do they need to be ended manually?
There was a problem hiding this comment.
Ack on process.exitCode, sounds like it will be safer.
I'm not sure if we need to manually end the stream. This is legacy code. What would end the streams otherwise, though?
There was a problem hiding this comment.
I would need to test this to make sure, but just a normal pipe should work:
c.stdout.pipe(stdoutStream);
c.stderr.pipe(stderrStream);
There was a problem hiding this comment.
I don't think we can do that. We need things as-they-are in order to:
- Send output to stdio in real-time
- Save that output in a file (for when
exec-child.jsfinishes)
If we pipe the output, then we can't achieve bullet 2.
There was a problem hiding this comment.
You can just pipe twice:
c.stdout.pipe(fs.createWriteStream(stdoutFile));
c.stderr.pipe(fs.createWriteStream(stderrFile));
c.stdout.pipe(process.stdout);
c.stderr.pipe(process.stderr);
There was a problem hiding this comment.
Nevermind, I was confused. You're right 👍
|
Comments are addressed, I'll look into the CI failures |
Codecov Report
@@ Coverage Diff @@
## master #791 +/- ##
=========================================
+ Coverage 95.5% 95.81% +0.3%
=========================================
Files 34 34
Lines 1269 1361 +92
=========================================
+ Hits 1212 1304 +92
Misses 57 57
Continue to review full report at Codecov.
|
|
@freitagbr this is ready for review (tests should pass, the last commit is just a comment) |
|
For reference, I found a few commits related to the The way we have it currently is basically identical to piping without |
src/exec-child.js
Outdated
| // child_process could not run the command. | ||
| /* istanbul ignore next */ | ||
| process.exitCode = 127; | ||
| } |
There was a problem hiding this comment.
If childProcess.exec throws, then c will be undefined, right? That means the code below this try/catch will fail.
There was a problem hiding this comment.
I think you're right. We should actually think of valid cases which might trigger this and document them. I think maxBuffer normally triggers this, but we're hitting the maxBuffer on the execFileSync() call in exec.js first.
There was a problem hiding this comment.
Looking at the current code for childProcess.exec, I'm not sure it can throw synchronous exceptions. Not sure about past versions of node, however.
There was a problem hiding this comment.
Hmmm you may be right.
I tried adding all sorts of broken/invalid cases and couldn't actually trigger this.
There was a problem hiding this comment.
Just for the sake of verifying, I looked through the history of childProcess.exec and it hasn't changed much in 2 years, basically since v0.12. So, it is safe to remove the try/catch
Thanks for researching. Looking back, we used to pipe stdout & stderr into the same file (externally visible as |
|
@freitagbr I think everything is resolved. |
|
LGTM, I restarted the Travis builds for good measure. |
|
Travis had problems last night 😞 |
This parameter isn't needed, we can easily rely on exit code status for this.
Eliminating the parameter reduces file IO, code complexity, and removes a busy
loop.
Issue #782