Remove separate "internal error" from exec #802

freitagbr · 2017-11-10T21:15:57Z

Fixes #734

Removes common.error('internal error') from exec, allowing any internal errors in exec to be wrapped by ShellJSInternalError instead.

nfischer · 2017-11-11T21:22:00Z

test/exec.js

@@ -35,7 +35,7 @@ test('config.fatal and unknown command', t => {
  shell.config.fatal = true;
  t.throws(() => {
    shell.exec('asdfasdf'); // could not find command
-  }, /exec: internal error/);
+  }, /((asdfasdf: not found)|(asdfasdf: command not found)|('asdfasdf' is not recognized))/);


Why 3 regexes? If these are different shells, let's put a comment.

Should we check it's a shelljsinternalerror?

nfischer · 2017-11-11T21:23:18Z

test/exec.js

-  t.truthy(shell.error());
+  const err = t.throws(() => {
+    shell.exec(`${JSON.stringify(shell.config.execPath)} test/resources/exec/slow.js 100`, { timeout: 10 }); // times out
+  }, /ENOENT: no such file or directory/);


Oh, let's actually debug this. This looks like a real bug

nfischer · 2017-11-11T21:25:13Z

Wow, this is a huge improvement. For curiosity, what do the stack traces look like for these errors?

The stdout and stderr files may never be opened or written to in certain circumstances. In particular, if the timeout is short enough, the child node process does not have enough time to start, and the child script does not execute, so the files are not written to. So, catch errors form trying to read the files, and ignore them.

nfischer · 2017-11-16T05:56:51Z

src/exec.js

  if (opts.encoding === 'buffer') {
-    stdout = fs.readFileSync(stdoutFile);
-    stderr = fs.readFileSync(stderrFile);
+    try { stdout = fs.readFileSync(stdoutFile); } catch (e) {}


Is this really the right fix? Why aren't the files created by the child process? I think that's where the bug really is.

In the unit test specifically, the timeout is set to 10 ms, and node doesn't startup in that time. The code in exec-child.js is what is creating the stdout and stderr files, and so if node hasn't started, then the files will not exist after the execSync call exits.

Perhaps instead the files should be created before the execSync call, and the child process just writes to them?

This sort of bug usually means that the exec() call inside exec-child.js throws an exception:

shelljs/src/exec-child.js

Line 19 in 8ab0a3a

var c = childProcess.exec(cmd, execOptions, function (err) {

Oh, didn't see your last message. Huh, if node doesn't startup, then I think we have a few options:

Don't try to handle this, let it be an internal error

Modify the test's timeout so that we're more confident node can start up

Create files beforehand (but this isn't really necessary in the common case, seems like a waste)

This test case is all about checking the timeout option works. So we can also try something like:

echo start sleep 5 echo stop # Check that this doesn't print, because we timeout before this

Sorry for the delay, I'll work on this soon.

No rush. I think it's fine to wrap this into v0.9 anyway

The test passes when the timeout is set to 500 ms (minus the try/catch statements).If you think it's alright, I will just increase the timeout on the test.

You mean to revert the try-catch statements here and to change the test timeout instead? I think that's fine.

Ok, I'll go ahead with this.

nfischer · 2018-01-12T06:32:22Z

test/exec.js

-  }, /exec: internal error/);
+    // the expected message depends on where tests are run:
+    //   /bin/sh says "foo: not found"
+    //   /bin/bash says "foo: command not found"


We only need to support /bin/sh and cmd.exe, since these are the default shells. I think error messages are implementation specific, so let's just limit it to those 2 if we can get away with it (so long as mac, linux, and windows bots are happy).

On my machine, at least, /bin/sh is the default shell, but the message is different than /bin/sh on linux.

How about we regex for asdfasdf instead? I think that's good enough.

nfischer · 2018-01-12T06:35:04Z

src/exec.js

  if (opts.encoding === 'buffer') {
-    stdout = fs.readFileSync(stdoutFile);
-    stderr = fs.readFileSync(stderrFile);
+    try { stdout = fs.readFileSync(stdoutFile); } catch (e) {}


You mean to revert the try-catch statements here and to change the test timeout instead? I think that's fine.

nfischer · 2018-01-28T08:22:44Z

@freitagbr any idea why we have travis failures?

nfischer · 2018-01-28T08:34:27Z

Ah, I tried it locally. It looks like e.status is 0. Must be a behavior change between node 5/6. We probably shouldn't let code be 0 if an exception was thrown. Maybe we can try code = e.status || DEFAULT_ERROR_CODE.

codecov-io · 2018-01-29T05:48:56Z

Codecov Report

Merging #802 into master will increase coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage   95.47%   95.81%   +0.33%     
==========================================
  Files          34       34              
  Lines        1260     1361     +101     
==========================================
+ Hits         1203     1304     +101     
  Misses         57       57

Impacted Files	Coverage Δ
src/exec.js	`97.1% <100%> (+0.04%)`	⬆️
src/common.js	`98.93% <0%> (+0.58%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6189d7f...5e79a99. Read the comment docs.

nfischer · 2018-01-29T19:07:48Z

LGTM. Could you add a comment explaining that .status is sometimes 0, even when this is in fact an error, and that DEFAULT_ERROR_CODE was chosen arbitrarily?

freitagbr · 2018-01-29T19:10:58Z

Sure, I will do that. Also, this is a breaking change, so do we want to land this on a development branch? i.e. landing in v0.9 vs. v0.8.

nfischer · 2018-01-29T20:23:33Z

I don't think this is really breaking anything. The behavior changes I see are:

exec() used to abort in an unhelpful way when it had bugs. The bugs are still there, but now it aborts in a helpful way
- If someone depended on aborting-without-exception and are now broken, it means they were doing something which caused an exec() bug to begin with. We don't support bugs
exec() used to return a shellstring with an empty .stderr attribute. Now its .stderr is useful

If you agree, feel free to merge

joelchoo · 2018-05-08T04:14:54Z

@nfischer I'm really interested in getting this fix. Any indication of when 0.8.2 will be coming out?

Remove separate "internal error" from exec

54edcae

freitagbr added breaking Breaking change exec Issues specific to the shell.exec() API labels Nov 10, 2017

freitagbr added this to the v0.8.0 milestone Nov 10, 2017

freitagbr assigned nfischer Nov 10, 2017

freitagbr requested a review from nfischer November 10, 2017 21:15

Fix unknown command error regex

d588c63

nfischer requested changes Nov 11, 2017

View reviewed changes

freitagbr added 2 commits November 15, 2017 20:56

Add message about command not found regex

cff9fdc

nfischer reviewed Nov 16, 2017

View reviewed changes

nfischer modified the milestones: v0.8.0, v0.9.0 Jan 11, 2018

nfischer requested changes Jan 12, 2018

View reviewed changes

freitagbr added 2 commits January 11, 2018 22:41

Do not silence errors due to short timeouts

8201a60

Simplify test regex for missing command

e60e3b0

Default error code to 1 if not set

5e79a99

nfischer approved these changes Jan 29, 2018

View reviewed changes

nfischer removed the breaking Breaking change label Jan 29, 2018

freitagbr merged commit 9077f41 into master Jan 29, 2018

freitagbr deleted the exec-internal-error branch January 29, 2018 20:28

nfischer mentioned this pull request Feb 7, 2018

shell.exec() doesn't respond correctly to config.fatal = true #735

Closed

nfischer modified the milestones: v0.9.0, v0.8.x Jul 12, 2018

This was referenced Nov 29, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 sharksandcupcakes/html-inspector#1

Open

[Snyk] Upgrade shelljs from 0.8.2 to 0.8.3 dyna-dot/angular#4

Open

snyk-bot mentioned this pull request Dec 18, 2019

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 BeeTech-global/generator-api-ecma6#5

Open

snyk-bot mentioned this pull request Mar 8, 2020

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 michelvankessel/colored-coins-copay-wallet#1

Open

This was referenced Mar 24, 2020

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 DeviaVir/zenbot#2006

Merged

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 DeviaVir/zenbot#2010

Merged

snyk-bot mentioned this pull request Apr 3, 2020

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 telegraph/api-responder#2

Open

This was referenced Apr 24, 2020

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 electrode-io/electrode-gulp-helper#3

Open

[Snyk] Upgrade shelljs from 0.7.8 to 0.8.3 electrode-io/electrode-electrify#31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove separate "internal error" from exec #802

Remove separate "internal error" from exec #802

freitagbr commented Nov 10, 2017

nfischer Nov 11, 2017

nfischer Nov 11, 2017

nfischer commented Nov 11, 2017

nfischer Nov 16, 2017

freitagbr Nov 16, 2017

nfischer Nov 16, 2017

nfischer Nov 16, 2017

nfischer Nov 16, 2017

freitagbr Jan 11, 2018

nfischer Jan 11, 2018

freitagbr Jan 12, 2018

nfischer Jan 12, 2018

freitagbr Jan 12, 2018

nfischer Jan 12, 2018

freitagbr Jan 12, 2018 •

edited

nfischer Jan 12, 2018

nfischer Jan 12, 2018

nfischer commented Jan 28, 2018

nfischer commented Jan 28, 2018

codecov-io commented Jan 29, 2018 •

edited

nfischer commented Jan 29, 2018

freitagbr commented Jan 29, 2018

nfischer commented Jan 29, 2018

joelchoo commented May 8, 2018

Remove separate "internal error" from exec #802

Remove separate "internal error" from exec #802

Conversation

freitagbr commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfischer commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freitagbr Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfischer commented Jan 28, 2018

nfischer commented Jan 28, 2018

codecov-io commented Jan 29, 2018 • edited

Codecov Report

nfischer commented Jan 29, 2018

freitagbr commented Jan 29, 2018

nfischer commented Jan 29, 2018

joelchoo commented May 8, 2018

freitagbr Jan 12, 2018 •

edited

codecov-io commented Jan 29, 2018 •

edited