Add boolean `fatal` option to `exec()` function #961

WesCossick · 2019-08-12T14:54:51Z

This PR introduces a new boolean fatal option for the exec() function. Like the existing silent option, this new option allows you to override the global fatal configuration parameter on a per-command basis, like:

exec('example command', {
    fatal: true|false,
});

This eliminates the need for the following pattern:

const oldFatal = shell.config.fatal;
shell.config.fatal = false;
exec('some command that might fail');
shell.config.fatal = oldFatal;

The old pattern can introduce race conditions in async functions, an issue that was surfaced in our code with the new ESLint rule, require-atomic-updates.

WesCossick · 2019-08-12T14:55:24Z

Once I receive feedback on the implementation itself, I can add documentation and tests!

nfischer

Left some comments, but mostly good (and this seems like a good feature to add to the project)

src/exec.js

nfischer · 2019-08-13T07:15:40Z

src/exec.js

@@ -28,6 +28,7 @@ function execSync(cmd, opts, pipe) {

  opts = common.extend({
    silent: common.config.silent,
+    fatal: common.config.fatal,


This is also redundant (see comment below)

nfischer · 2019-08-13T07:17:34Z

src/common.js

@@ -118,7 +118,7 @@ function error(msg, _code, options) {
  state.error += logEntry;

  // Throw an error, or log the entry
-  if (config.fatal) throw new Error(logEntry);
+  if (config.fatal && options.fatal !== false) throw new Error(logEntry);


Instead of this, how about we assign fatal: config.fatal up above in DEFAULT_OPTIONS?

Fixed it; not sure how I missed that pattern originally.

You should update this to check options.fatal instead of config.fatal (otherwise, this should be OK)

nfischer · 2019-08-13T07:23:29Z

Here's a similar test you might be able to base this on:

shelljs/test/config.js

Lines 42 to 48 in 57df38c

    
           test.cb('config.fatal = true', t => { 
        
             const script = 'require(\'./global.js\'); config.silent=true; config.fatal=true; cp("this_file_doesnt_exist", "."); echo("got here");'; 
        
             utils.runScript(script, (err, stdout) => { 
        
               t.falsy(stdout.match('got here')); 
        
               t.end(); 
        
             }); 
        
           });

codecov-io · 2019-08-14T22:18:47Z

Codecov Report

Merging #961 into master will decrease coverage by 0.13%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
- Coverage   97.28%   97.14%   -0.14%     
==========================================
  Files          34       34              
  Lines        1289     1298       +9     
==========================================
+ Hits         1254     1261       +7     
- Misses         35       37       +2

Impacted Files	Coverage Δ
src/common.js	`98.46% <100%> (ø)`	⬆️
src/exec.js	`94.87% <83.33%> (-2.23%)`	⬇️

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 57df38c...a9d76ad. Read the comment docs.

WesCossick · 2019-08-14T23:45:43Z

There is a very strange error where one of the new tests I added causes common.js to die mysteriously on line 119.

I drilled it down to that line by adding a throw new Error('TEST'); line before and after that line and running npm test for each. When the error is thrown after line 119, it never gets thrown for that one test that's failing.

I have no idea why that is.

nfischer · 2019-08-15T04:58:29Z

Not sure what the failure on 119 is (I don't see it on the bots), but https://travis-ci.org/shelljs/shelljs/jobs/572075940 failed here:

exec › options.fatal = true and unknown command
  /home/travis/build/shelljs/shelljs/test/exec.js:51
   50:   shell.config.fatal = false;                                         
   51:   t.throws(() => {                                                    
   52:     shell.exec('asdfasdf', { fatal: true }); // could not find command

This makes sense, because the implementation has a bug (per my previous comment, you need to check options.fatal rather than config.fatal in the code).

WesCossick · 2019-08-15T13:18:24Z

The code does check options.fatal: https://github.com/WesCossick/shelljs/blob/master/src/common.js#L122

WesCossick · 2019-08-15T13:34:45Z

When running npm test, the options.fatal = true and unknown command test dies at common.js precisely on line 119. No code after that point executes, and I have no idea why. But, just running the package normally, it's clear that what's really happening is that the wrap() function relies on checking config.fatal before deciding whether to throw an error (this line).

How do you propose this should be addressed? There does not appear to be an easy way to wire the new fatal option through to the wrap() function without using the global config.

WesCossick · 2019-08-21T19:45:18Z

I've implemented what I think is a decent solution, working within this project's existing design.

I've introduced a new handlesFatalDynamically option for the common wrap() function. Functions like exec can then use this boolean to indicate to the wrap() function that they suppress their own errors dynamically. This means that when they throw an error, it's because they want it to be seen, regardless of the global fatal value.

Let me know your thoughts, @nfischer.

nfischer · 2019-08-23T21:15:54Z

src/common.js

@@ -88,7 +88,7 @@ CommandError.prototype = Object.create(Error.prototype);
 CommandError.prototype.constructor = CommandError;
 exports.CommandError = CommandError; // visible for testing

-// Shows error message. Throws if config.fatal is true
+// Shows error message. Throws if fatal is true (defaults to config.fatal, overridable with options.fatal)


nit: "Throws if options.fatal is..."

The verbiage here is intentionally just "fatal" because it's looking at two separate values. First it looks at the global config, but it also can be overridable with the function's options. The options.fatal value isn't always provided, in which case, it's really the global config that determines the behavior.

I suppose you could rewrite it to read: // Throws if options.fatal is true, or if options.fatal is undefined and config.fatal is true. Let me know if you think that wording is more clear.

nfischer · 2019-08-23T21:17:22Z

src/exec.js

 });

 // We use this function to run `exec` synchronously while also providing realtime
 // output.
 function execSync(cmd, opts, pipe) {
  if (!common.config.execPath) {
-    common.error('Unable to find a path to the node binary. Please manually set config.execPath');
+    common.error('Unable to find a path to the node binary. Please manually set config.execPath', { continue: true, fatal: opts.fatal });


Why continue: true?

Without it, even if fatal: false is provided to the common error() function, it will still throw a CommandError if continue is not true. See: https://github.com/WesCossick/shelljs/blob/master/src/common.js#L125

Normally, the wrap() function would have caught that error, seen that fatal was disabled globally, and not re-thrown it. But now, the wrap() function will always throw errors since handlesFatalDynamically: true for the exec command. That means it's up to the exec command to decide whether to throw it or not. Even if it doesn't throw an error, at a minimum it should not execute any logic past that point.

To make this more clear, I've re-written this bit of code (here and the other place where continue: true was followed by a return.

nfischer · 2019-08-23T21:19:14Z

src/exec.js

    async: false,
  }, options);

+  if (!command) {
+    common.error('must specify command', { continue: true, fatal: options.fatal });
+    return;


It doesn't look right for continue: true to be followed by return...

See above comment.

test/exec.js

src/common.js

WesCossick · 2019-08-27T21:27:36Z

Hey @nfischer, let me know what you think of the recent changes.

WesCossick · 2019-09-20T16:00:04Z

@nfischer Any update on this?

src/common.js

Add boolean fatal option to exec() function

14aee67

nfischer self-requested a review August 13, 2019 07:09

nfischer reviewed Aug 13, 2019

View reviewed changes

nfischer added exec Issues specific to the shell.exec() API feature labels Aug 13, 2019

WesCossick added 4 commits August 14, 2019 17:00

Clean up implementation of default fatal option

00a3045

Leave TODO note for @nfischer

96b3676

Add reason for TODO to note

0191691

Fix incorrect object reference

f6fbc1a

WesCossick added 2 commits August 14, 2019 18:20

Add tests for new exec option

cd62965

Update comments and docs

df7487b

WesCossick added 3 commits August 21, 2019 14:29

Introduce new handlesFatalDynamically option for wrap()

8a4ea52

Remove trailing space

2e8ba70

Fix last failing test

c6e52fa

Fix test on Windows

c1e594f

nfischer reviewed Aug 23, 2019

View reviewed changes

WesCossick added 3 commits August 27, 2019 16:05

Add TODO note for @nfischer

d219824

Simplify assertion

d4ebdb8

Rewrite two error statements to avoid continue: true

a9d76ad

nfischer approved these changes Oct 2, 2019

View reviewed changes

src/common.js Show resolved Hide resolved

nfischer merged commit 4e38240 into shelljs:master Oct 2, 2019

vilaboim mentioned this pull request Oct 4, 2019

Launch a release to include #961 #968

Closed

nfischer added this to the v0.9.0 milestone Oct 8, 2019

nfischer mentioned this pull request Dec 19, 2019

shell.exec() ignores fatal option when not set globally #981

Closed

Add boolean fatal option to exec() function #961

Add boolean fatal option to exec() function #961

Uh oh!

Conversation

WesCossick commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WesCossick commented Aug 12, 2019

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfischer commented Aug 13, 2019

Uh oh!

codecov-io commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

WesCossick commented Aug 14, 2019

Uh oh!

nfischer commented Aug 15, 2019

Uh oh!

WesCossick commented Aug 15, 2019

Uh oh!

WesCossick commented Aug 15, 2019

Uh oh!

WesCossick commented Aug 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WesCossick Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WesCossick commented Aug 27, 2019

Uh oh!

WesCossick commented Sep 20, 2019

Uh oh!

Uh oh!

Uh oh!

Add boolean `fatal` option to `exec()` function #961

Add boolean `fatal` option to `exec()` function #961

WesCossick commented Aug 12, 2019 •

edited

Loading

codecov-io commented Aug 14, 2019 •

edited

Loading

WesCossick Aug 27, 2019 •

edited

Loading