Fix: avoid using `process.exit()` when possible #3955

BYK · 2017-07-18T16:50:15Z

Summary

Refs #3524. We are using process.exit() pretty liberally although it is strongly advised not to use it since it may cause the stdout to get corrupted/terminated before a full flush. This patch changes all possible process.exit(code) calls with process.exitCode = code statements.

We'd also ideally enable no-process-exit rule in ESLint but it requires an upgrade to ESLint v4 which should be handled separately.

Test plan

Expect tests to pass and actually finish (not run indefinitely due to yarn not exiting at all). Also, the script referenced in #3524 to output something like the following:

DATA 18
DATA 8192
DATA 8192
DATA 5392
DATA 15
EXIT 0

arcanis · 2017-07-18T16:54:28Z

src/cli/index.js

@@ -142,7 +142,7 @@ if (outputWrapper) {
 if (command.noArguments && commander.args.length) {
  reporter.error(reporter.lang('noArguments'));
  reporter.info(command.getDocsInfo);
-  process.exit(1);
+  process.exitCode = 1;


You need to shortcut the rest of the code to have the same behavior - process.exit doesn't return, but setting process.exitCode will continue executing the rest of the code.

Not shortcutting the whole point with this change. We should not need the shortcut behavior and turns out we really don't since all tests are passing. It was only necessary at one point so I left it that way.

I don't think that's a good idea.

it kinda violates the principle of the least surprise, since we would no longer have the guarantee that any part of this file would only be executed if no other command matched. It's only a matter of time before we make some mistake that will execute two commands in some edge case.

Case in point, your current code has a bug that will make yarn -v print the version number before running yarn install -v (and yarn -v -h will print the version number then call yarn help -v).

Even without that, not shortcutting also means that we will be running useless tasks (such as instanciating a reporter etc) even when not needed. That's an issue because those commands must be fast (some tools such as Nuclide might need to call them often, and if their results are late it will slow down the all process).

I'm not against using exitCode, you have a point that shutting down the program before the I/O has finished is bad, but I strongly think we then need to refactor the CLI code to make sure we never have to check that our code won't get called twice - might only be a matter of adding else clauses 😃

arcanis · 2017-07-18T16:55:19Z

src/util/signal-handler.js

@@ -3,7 +3,7 @@ import {forwardSignalToSpawnedProcesses} from './child.js';

 function forwardSignalAndExit(signal: string) {
  forwardSignalToSpawnedProcesses(signal);
-  process.exit(1);
+  throw new Error('Received SIGTERM, terminating.');


Will that print a stacktrace on the console? That won't be helpful for the user.

I thought it would be. Why do you think it wouldn't?

SIGTERM is the default signal sent by kill. It doesn't make much sense to print a stacktrace (which heavily imply that something went wrong in the program implementation) if the termination came from the outside, don't you think?

Alright, makes sense. I'll find an alternative way for this one.

arcanis · 2017-07-19T11:50:28Z

src/cli/index.js

@@ -142,7 +142,7 @@ if (outputWrapper) {
 if (command.noArguments && commander.args.length) {
  reporter.error(reporter.lang('noArguments'));
  reporter.info(command.getDocsInfo);
-  process.exit(1);
+  process.exitCode = 1;


I don't think that's a good idea.

it kinda violates the principle of the least surprise, since we would no longer have the guarantee that any part of this file would only be executed if no other command matched. It's only a matter of time before we make some mistake that will execute two commands in some edge case.

Case in point, your current code has a bug that will make yarn -v print the version number before running yarn install -v (and yarn -v -h will print the version number then call yarn help -v).

Even without that, not shortcutting also means that we will be running useless tasks (such as instanciating a reporter etc) even when not needed. That's an issue because those commands must be fast (some tools such as Nuclide might need to call them often, and if their results are late it will slow down the all process).

I'm not against using exitCode, you have a point that shutting down the program before the I/O has finished is bad, but I strongly think we then need to refactor the CLI code to make sure we never have to check that our code won't get called twice - might only be a matter of adding else clauses 😃

arcanis · 2017-07-19T13:25:30Z

src/cli/index.js

+  loudRejection();
+  handleSignals();
+
+  const startArgs = process.argv.slice(0, 2);


What do you think about modularizing and putting this in the main parameters? I can see this function as an interesting entry point to run commands in our testcases - we currently need to instanciate each class and pass them the right properties, which might be tedious and error-prone, so it could be nice to just be able to use run([ 'install', '--check-files' ]) instead and use the actual CLI pipeline.

Actually my comment was more about Line 31, but it's also applicable here.

arcanis · 2017-07-20T12:14:46Z

src/reporters/console/console-reporter.js

@@ -196,7 +196,7 @@ export default class ConsoleReporter extends BaseReporter {
        (err, answer) => {
          if (err) {
            if (err.message === 'canceled') {
-              process.exit(1);


Shouldn't we exit immediately here? Or reject?

Yeah, I think you're right. I thought about the same thing and don't know why I didn't merge the reject part. Update coming.

arcanis · 2017-07-20T12:15:21Z

scripts/build-webpack.js

@@ -35,6 +35,8 @@ const compiler = webpack({
  output: {
    filename: `yarn-${version}.js`,
    path: path.join(basedir, 'artifacts'),
+    library: 'yarn-cli',
+    libraryTarget: 'commonjs2',


Is this change required? What breaks without it?

Yes, this is the solution that fixes the single-file bundles. Without this, Webpack exports an empty object, causing yarn-bundled-entry-point.js to fail since main function can't be imported.

**Summary** Upgrade ESLint to version 4. I did this mostly to get [no-process-exit](http://eslint.org/docs/rules/no-process-exit) rule introduced in v4. Refs #3955. **Test plan** Lint should pass without errors.

**Summary** A follow up to #3955 to disable further usage of `process.exit()`. **Test plan** `yarn lint` should not produce any errors.

skevy · 2017-07-29T17:42:28Z

I noticed when pulling down master and running build-dist that the webpack builds were no longer working.

I traced it back to this PR. You can see in CircleCI the output of the last working build here:

https://circleci.com/gh/yarnpkg/yarn/4533

Notice how $ ./artifacts/yarn-`./dist/bin/yarn --version`.js --version outputs a version number.

When this commit runs in CI, however (https://circleci.com/gh/yarnpkg/yarn/4536), there is no version number output, and I confirm that the webpack build does not work at all.

cc @BYK @arcanis @Daniel15

Daniel15 · 2017-07-29T17:51:25Z

Hmm, interesting, I wonder why the nightly end-to-end tests didn't fail. They use the nightly Debian packages, which contain the Webpack build inside them. https://build.dan.cx/job/yarn-e2e/label=docker,os=ubuntu-16.04/269/console

skevy · 2017-07-29T18:07:08Z

I'm not sure why those don't fail (without looking into it more) -- but the reason CircleCI doesn't fail is because yarn --version still exists with code 0, even though it doesn't do anything.

Daniel15 · 2017-07-29T18:08:24Z

Right. Maybe the build should assert that it actually returns the correct version number.

skevy · 2017-07-29T18:10:08Z

👍

BYK · 2017-07-31T11:45:10Z

I'll work on this soon.

BYK · 2017-07-31T18:19:05Z

Filed #4057, actively working on it.

BYK requested a review from arcanis July 18, 2017 16:50

BYK assigned arcanis Jul 18, 2017

arcanis requested changes Jul 18, 2017

View reviewed changes

BYK mentioned this pull request Jul 19, 2017

Update: upgrade ESLint to 4.3.0 #3962

Merged

arcanis requested changes Jul 19, 2017

View reviewed changes

arcanis reviewed Jul 19, 2017

View reviewed changes

arcanis approved these changes Jul 19, 2017

View reviewed changes

BYK force-pushed the stdout-buffer branch from 6ad73b7 to bde7b57 Compare July 19, 2017 15:32

arcanis reviewed Jul 20, 2017

View reviewed changes

arcanis approved these changes Jul 20, 2017

View reviewed changes

Fix: avoid using process.exit() when possible

579d32f

BYK force-pushed the stdout-buffer branch from a231d62 to 579d32f Compare July 20, 2017 15:30

BYK merged commit 61e402b into master Jul 20, 2017

BYK deleted the stdout-buffer branch July 20, 2017 16:23

BYK pushed a commit that referenced this pull request Jul 26, 2017

Chore: enable no-process-exit lint rule

4e3f963

**Summary** A follow up to #3955 to disable further usage of `process.exit()`. **Test plan** `yarn lint` should not produce any errors.

BYK mentioned this pull request Jul 26, 2017

Chore: enable no-process-exit lint rule #4026

Merged

BYK added a commit that referenced this pull request Jul 27, 2017

Chore: enable no-process-exit lint rule (#4026)

357dc22

**Summary** A follow up to #3955 to disable further usage of `process.exit()`. **Test plan** `yarn lint` should not produce any errors.

BYK mentioned this pull request Jul 31, 2017

Single bundle builds are broken #4057

Closed

Fix: avoid using process.exit() when possible #3955

Fix: avoid using process.exit() when possible #3955

Uh oh!

Conversation

BYK commented Jul 18, 2017

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!

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!

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!

BYK Jul 20, 2017 • 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!

skevy commented Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Daniel15 commented Jul 29, 2017

Uh oh!

skevy commented Jul 29, 2017

Uh oh!

Daniel15 commented Jul 29, 2017

Uh oh!

skevy commented Jul 29, 2017

Uh oh!

BYK commented Jul 31, 2017

Uh oh!

BYK commented Jul 31, 2017

Uh oh!

Uh oh!

Fix: avoid using `process.exit()` when possible #3955

Fix: avoid using `process.exit()` when possible #3955

BYK Jul 20, 2017 •

edited

Loading

skevy commented Jul 29, 2017 •

edited

Loading