NotificationsYou must be signed in to change notification settings
Fork744
Star14.4k

Remove codeFile parameter#791

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nfischer merged 5 commits intomasterfromremove-codefile-param

Oct 31, 2017

Merged

Remove codeFile parameter#791

nfischer merged 5 commits intomasterfromremove-codefile-param

Oct 31, 2017

Conversation

Copy link

Member

nfischer commentedOct 20, 2017

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

Remove codeFile parameter

a8d80f3

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 busyloop.Issue#782

nfischer added exec

Issues specific to the shell.exec() API

refactor labels

Oct 20, 2017

nfischer self-assigned this

Oct 20, 2017

nfischer requested a review fromfreitagbr

October 20, 2017 04:28

Fix lint error

94f57ac

freitagbr reviewed

Oct 20, 2017

View reviewed changes

src/exec-child.js Outdated

		});
		}catch(e){
		// child_process could not run the command.
		process.exit(127);

Copy link

Contributor

freitagbrOct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 settingprocess.exitCode is a safer way to do this.

Copy link

Contributor

freitagbrOct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Also, below this, we are manually endingstdoutStream andstderrStream whenc.stdout andc.stderr end. Why do they need to be ended manually?

Copy link

MemberAuthor

nfischerOct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ack onprocess.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?

Copy link

Contributor

freitagbrOct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I would need to test this to make sure, but just a normalpipe should work:

c.stdout.pipe(stdoutStream);c.stderr.pipe(stderrStream);

Copy link

MemberAuthor

nfischerOct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 whenexec-child.js finishes)

If we pipe the output, then we can't achieve bullet 2.

Copy link

Contributor

freitagbrOct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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);

Copy link

MemberAuthor

nfischerOct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nevermind, I was confused. You're right 👍

Remove legacy code, make code safer

893eec6

Copy link

MemberAuthor

nfischer commentedOct 27, 2017

Comments are addressed, I'll look into the CI failures

Copy link

codecov-io commentedOct 27, 2017•
edited
Loading

Codecov Report

Merging#791 intomaster willincrease coverage by0.3%.
The diff coverage is100%.

@@            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

Impacted Files	Coverage Δ
src/exec.js	`97.89% <100%> (+0.52%)`	⬆️
src/exec-child.js	`100% <100%> (ø)`	⬆️
src/common.js	`98.79% <0%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update9e3f9ab...63b5e21. Read thecomment docs.

Ignore coverage for exception

d1951c1

Copy link

MemberAuthor

nfischer commentedOct 27, 2017

@freitagbr this is ready for review (tests should pass, the last commit is just a comment)

nfischer mentioned this pull request

Oct 27, 2017

Harden shell.exec by writing the child process in a source file#782

Closed

Copy link

Contributor

freitagbr commentedOct 27, 2017•
edited
Loading

For reference, I found a few commits related to the{ end: false } option passed for the pipes:

The way we have it currently is basically identical to piping without{ end: false }, so I'm not sure if the original issues will crop up again. Perhaps it was an issue with node itself.

freitagbr reviewed

Oct 27, 2017

View reviewed changes

src/exec-child.js Outdated

		// child_process could not run the command.
		/* istanbul ignore next */
		process.exitCode=127;
		}

Copy link

Contributor

freitagbrOct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

IfchildProcess.exec throws, thenc will beundefined, right? That means the code below this try/catch will fail.

Copy link

MemberAuthor

nfischerOct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think you're right. We should actually think of valid cases which might trigger this and document them. I thinkmaxBuffer normally triggers this, but we're hitting themaxBuffer on theexecFileSync() call inexec.js first.

Copy link

Contributor

freitagbrOct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looking at the current code forchildProcess.exec, I'm not sure it can throw synchronous exceptions. Not sure about past versions of node, however.

Copy link

MemberAuthor

nfischerOct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hmmm you may be right.

I tried adding all sorts of broken/invalid cases and couldn't actually trigger this.

Copy link

MemberAuthor

nfischerOct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Removed it.

Copy link

Contributor

freitagbrOct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just for the sake of verifying, I looked through the history ofchildProcess.exec and it hasn't changed much in 2 years, basically sincev0.12. So, it is safe to remove thetry/catch

Copy link

MemberAuthor

nfischer commentedOct 28, 2017

The way we have it currently is basically identical to piping without { end: false }, so I'm not sure if the original issues will crop up again. Perhaps it was an issue with node itself.

Thanks for researching. Looking back, we used to pipe stdout & stderr into the same file (externally visible asshell.exec().output). I changed thisa while back to separate the two streams (so we have.stdout and.stderr). Now that we have 2 readable streams piping into 2 writable streams, we can use default stream-closing behavior.