Handle scripts failing with exit signals (fixes: #3954) #3995

onurtemizkan · 2017-07-23T09:35:11Z

Summary
Fixes: #3954, Adds error handling for scripts that are failing with a signal such as SIGBUS, SIGSEGV, etc.

In current behavior, yarn checks the exit code of a script to show errors and fail the process. But certain errors make node child-processes to exit with a signal instead. (e.g. the case in #3954)

This fix adds support for handling exit signals from child processes and shows more informative error messages.

Note: This implementation handles exit signals priorly, assuming exit signals are pointing to more critical low-level issues.

Test plan

Added 3 test cases (one for zero exit code, one for non-zero exit code, one for exit signal)

BYK

Very nice fix, thank you!

Looks like you have a failing test to fix and my comments to address, mostly about Windows :)

BYK · 2017-07-23T13:39:37Z

__tests__/fixtures/lifecycle-scripts/script-segfault/package.json

@@ -0,0 +1,9 @@
+{
+  "scripts": {
+    "compile": "gcc segfault.c -g -o segfault",


😮 Will this work on AppVeyor (Windows)

I'm not sure if Windows has the concept of "exiting with a signal" so it may be better to disable this test on Windows.

BYK · 2017-07-23T13:44:17Z

src/util/child.js

+                `Output:\n${stdout.trim()}`,
+              ].join('\n'),
+            );
+            err.EXIT_SIGNAL = signal;


I'd still add EXIT_CODE property.

BYK · 2017-07-23T13:44:40Z

src/util/child.js

+          if (signal) {
+            err = new SpawnError(
+              [
+                'Command failed.',


Looks like this message generation part is repeating itself quite a bit. May be you can DRY this up a bit?

BYK

Let's also put a reference to the node.js issue number about this only happening on macOS so we can remove the workaround in the future?

BYK · 2017-07-25T10:58:23Z

__tests__/lifecycle-scripts.js

+test('should not show any error messages when script ends successfully', async () => {
+  const stdout = await execCommand('test', 'script-success');
+
+  expect(stdout).toMatch(/Done in /);


This text may disappear in the future. May be just do expect().not.toThrow?

BYK · 2017-07-25T10:59:38Z

__tests__/lifecycle-scripts.js

+});
+
+test('should show correct error message when the script ends with an exit code', async () => {
+  await execCommand('test', 'script-fail').catch(error => {


This test would pass if no error is thrown (if the script doesn't fail). I'd use expect().toThrow() or even better, toThrowErrorMatchingSnapshot()

BYK · 2017-07-25T11:00:03Z

__tests__/lifecycle-scripts.js

+});
+
+test('should show correct error message when the script ends an exit signal', async () => {
+  await execCommand('test', 'script-segfault').catch(error => {


Same as above, use toThrow*()

BYK · 2017-07-25T11:00:54Z

src/util/execute-lifecycle-script.js

@@ -268,7 +268,11 @@ export async function execCommand(stage: string, config: Config, cmd: string, cw
    return Promise.resolve();
  } catch (err) {
    if (err instanceof SpawnError) {
-      throw new MessageError(reporter.lang('commandFailed', err.EXIT_CODE));
+      if (err.EXIT_SIGNAL) {


DRY this up.

onurtemizkan · 2017-07-26T22:07:28Z

@BYK while making updates on tests, I used resolves and rejects helpers, which are added at Jest v20.0.0. We are currently using Jest 20.0.4 but our flow-type definitions for Jest do not include those two properties. So the flow checks will fail on CI. Should I raise another issue for updating all the Jest flow types or is it enough to add only those two type defs with this PR?

onurtemizkan · 2017-08-01T22:26:23Z

__tests__/lifecycle-scripts.js

-    expect(error.code).not.toBe(0);
-  });
+test('should throw error when the script ends with an exit code', async () => {
+  await expect(execCommand('test', 'script-fail')).rejects.toBeDefined();


@BYK this test (also the test below) should fail when the script unexpectedly succeeds. This looked a good way to message-independently test the error to me. What do you think?

BYK · 2017-08-02T16:07:26Z

__tests__/lifecycle-scripts.js

@@ -142,3 +142,17 @@ test('should inherit existing environment variables when setting via yarnrc', as
  expect(stdout).toMatch(/^RAB$/m);
  expect(stdout).toMatch(/^FOO$/m);
 });
+
+test('should not show any error messages when script ends successfully', async () => {
+  await expect(execCommand('test', 'script-success')).resolves.toBeDefined();


You can simply return these, making it a one-liner:

test('should not show any error messages when script ends successfully', async () => expect(execCommand('test', 'script-success')).resolves.toBeDefined() );

@arcanis

**Summary** A combination of changes have caused `yarn upgrade-interactive` to exit with a promise rejection. In short, I believe it has always been a problem, but #3995 exposed it to the prompt. The child rejection inside of [upgrade-interactive](https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/upgrade-interactive.js#L152) is fine, as it was handled by the `Promise.race` condition; however, rejecting at the parent level inside of [console-reporter](https://github.com/yarnpkg/yarn/blob/master/src/reporters/console/console-reporter.js#L458) causes[ loud-rejection](https://github.com/sindresorhus/loud-rejection) to handle this. I believe @arcanis 's PR #4283 is what would allow us not to hook into `SIGINT` inside of the console reporter and allow the reporter to cleanly close itself. **Test Plan** Will work on some scenarios! This PR needs some more verification on my end ... @BYK @torifat @arcanis please jump in and provide any feedback you think could be helpful! Opened early for visibility :)

@arcanis

**Summary** A combination of changes have caused `yarn upgrade-interactive` to exit with a promise rejection. In short, I believe it has always been a problem, but yarnpkg#3995 exposed it to the prompt. The child rejection inside of [upgrade-interactive](https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/upgrade-interactive.js#L152) is fine, as it was handled by the `Promise.race` condition; however, rejecting at the parent level inside of [console-reporter](https://github.com/yarnpkg/yarn/blob/master/src/reporters/console/console-reporter.js#L458) causes[ loud-rejection](https://github.com/sindresorhus/loud-rejection) to handle this. I believe @arcanis 's PR yarnpkg#4283 is what would allow us not to hook into `SIGINT` inside of the console reporter and allow the reporter to cleanly close itself. **Test Plan** Will work on some scenarios! This PR needs some more verification on my end ... @BYK @torifat @arcanis please jump in and provide any feedback you think could be helpful! Opened early for visibility :)

BYK requested changes Jul 23, 2017

View reviewed changes

BYK self-assigned this Jul 23, 2017

onurtemizkan force-pushed the handle-segmentation-faults branch 2 times, most recently from e896b1b to e522f06 Compare July 23, 2017 20:44

BYK requested changes Jul 25, 2017

View reviewed changes

onurtemizkan added 2 commits August 1, 2017 23:20

Handle scripts failing with signals (fixes:yarnpkg#3954)

201ea6b

Review updates

9643175

onurtemizkan force-pushed the handle-segmentation-faults branch from e26019c to 9643175 Compare August 1, 2017 20:23

Review updates round yarnpkg#2

70e161c

onurtemizkan commented Aug 1, 2017

View reviewed changes

BYK approved these changes Aug 2, 2017

View reviewed changes

BYK merged commit 8ead83f into yarnpkg:master Aug 2, 2017

olingern mentioned this pull request Sep 13, 2017

Fix #4346 yarn upgrade interactive exit #4434

Merged

BYK mentioned this pull request Oct 30, 2017

A failure of yarn did not have an error shell return code #2698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle scripts failing with exit signals (fixes: #3954) #3995

Handle scripts failing with exit signals (fixes: #3954) #3995

onurtemizkan commented Jul 23, 2017

BYK left a comment

BYK Jul 23, 2017

BYK Jul 23, 2017

BYK Jul 23, 2017

BYK left a comment

BYK Jul 25, 2017

BYK Jul 25, 2017

BYK Jul 25, 2017

BYK Jul 25, 2017

onurtemizkan commented Jul 26, 2017 •

edited

Loading

onurtemizkan Aug 1, 2017 •

edited

Loading

BYK Aug 2, 2017

onurtemizkan Aug 2, 2017

Handle scripts failing with exit signals (fixes: #3954) #3995

Handle scripts failing with exit signals (fixes: #3954) #3995

Conversation

onurtemizkan commented Jul 23, 2017

BYK left a 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

BYK left a 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

onurtemizkan commented Jul 26, 2017 • edited Loading

onurtemizkan Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurtemizkan commented Jul 26, 2017 •

edited

Loading

onurtemizkan Aug 1, 2017 •

edited

Loading