Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exits with code 1 on SIGTERM without waiting for child #4667

Open
adamdunkley opened this issue Oct 9, 2017 · 9 comments

Comments

@adamdunkley
Copy link

commented Oct 9, 2017

Do you want to request a feature or report a bug?
This is a bug

What is the current behavior?
Currently yarn terminates with exit code 1 when it receives a SIGTERM without waiting for the child to exit and without passing its child's exit status up the chain.

The issue that this stemmed from was about handling SIGTERM events (#3424) and was introduced in pull request #3789.

If the current behavior is a bug, please provide the steps to reproduce.

Create a simple express script:

const app = require('./express');

const server = app.listen(config.port, () => {
    console.log(`server started`);
});
process.on('SIGTERM', () => {
    server.close(() => {
      setTimeout(() => {
        process.exit(0);
      }, 1000);
    });
});

Note that the script uses setTimeout to wait before exiting (as if it is waiting 1s for a client connection to end).

⇒  yarn start
yarn run v1.1.0
$ node src/index.js
server started

In another session run the following with the PID of that command:

kill $PID

Finally in that first session after it has exited:

⇒  echo $?
1

You can see that it exits immediately (not waiting 1s) and the error code is 1.

What is the expected behavior?

Expected behaviour is to do exactly as node would:

⇒  node src/index.js
server started
⇒  echo $?
0

This waits the 1s for the setTimeout (we kill using kill $PID in between the node and echo commands as above). You can also see we get the child's exit status (you can verify this by setting it to something else like process.exit(9) and seeing it come out).

I see some comments in the code where it is questioning what we should do about this:

// We want to exit immediately here since `SIGTERM` means that
// If we lose stdout messages due to abrupt exit, shoot the messenger?
process.exit(1); // eslint-disable-line no-process-exit

I think it's the child's job to decide what to do if a SIGTERM event is received. The parent should hand the signal to the child and wait to see if it exits. The only thing that I think Yarn should consider is whether there is a time limit that we would like to wait for naughty child processes that do not exit on time. Docker sets this to 10 seconds, whatever we set this to (if we do at all!!) we should make it a) clear and b) configurable.

One other thing: currently not sure what we do with SIGINT. I see weird stuff happening with that too (and I presume other signals?). Probably worth investigating signal handling more holistically?

Please mention your node.js, yarn and operating system version.

  • Node version: 7 & 8
  • Yarn version: 1.10.0
  • macOS 16.3.0 and alpine
@adamdunkley

This comment has been minimized.

Copy link
Author

commented Oct 9, 2017

I am very willing to commit time/code to getting this fixed properly as I think it's in a bit of a state at the moment but would like to get some agreement on direction first :)

@thetimothyp

This comment has been minimized.

Copy link

commented Dec 7, 2017

Has there been any more discussion on this?

I agree that it might be worth taking a look at signal handling as whole; it doesn't look like SIGINT is handled at all, apart from where onDeath() handles it in mutex mode. I think it might have to be handled slightly differently, since SIGINT is sent to all the processes in the foreground group instead of needing to be forwarded through the parent.

I'd also be willing to devote some time to this to draft an RFC or something like that.

@ianstormtaylor

This comment has been minimized.

Copy link

commented Feb 2, 2018

Any maintainer input on this? It seems like it prevents Yarn from being used for real in any situations where you need yarn run to spawn a child in the background, which would be very useful in CI.

@zargold

This comment has been minimized.

Copy link

commented May 23, 2018

I am still getting this issue by running a process using yarn vs running it with npm run:
package.json:

{
"scripts": {
"test": "concurrently -k --success first \"npm run dev:test\" \"yarn jest\""
}

app.js

process.on('SIGTERM', async () => {
  await server.close()
  console.log('EXITING GRACEFULLY...')
  process.exit(0)
})

WHEN USING YARN:
"test": "concurrently -k --success first \"yarn dev:test\" \"yarn jest\""

yarn test
....
[1] Test Suites: 3 passed, 3 total
[1] Tests:       42 passed, 42 total
[1] Snapshots:   0 total
[1] Time:        15.137s
[1] Ran all test suites.
[1] yarn jest exited with code 0
--> Sending SIGTERM to other processes..
[0] EXITING GRACEFULLY...
[0] yarn dev:test exited with code 1
✨  Done in 16.58s.

WHEN USING NPM RUN:

yarn test
...
[1] Test Suites: 3 passed, 3 total
[1] Tests:       42 passed, 42 total
[1] Snapshots:   0 total
[1] Time:        15.138s
[1] Ran all test suites.
[1] yarn jest exited with code 0
--> Sending SIGTERM to other processes..
[0] EXITING GRACEFULLY...
[0] npm run dev:test exited with code 0
✨  Done in 16.54s.

So with npm run running my server I can remove the --success first and it still passes but with yarn it fails due to the exit code 1 on the server.

System Info:

>>> yarn -v
1.6.0
>>> npm -v
5.6.0
>>> node -v
v10.0.0

Also I am not using nodemon to run my server:
"dev:test": "NODE_ENV=test node -r dotenv/config ./app.js",
"jest": "NODE_ENV=test jest --no-cache --runInBand",

@presidenten

This comment has been minimized.

Copy link

commented Nov 22, 2018

Same problem.

$ yarn -v
1.12.3

$ node -v
v8.12.0

$ npm -v
6.4.1

$ node index.js
^C

Doing async cleaning
Done

$ npm start
^C

Doing async cleaning


Doing async cleaning
Done

$ yarn start
yarn run v1.12.3
^C

Doing async cleaning

$ Done # <--- Console.log('Done') ends up on new line in console, because yarn already quit.

To test:
--package.json--

{
  "scripts": {
    "start": "node index.js"
  }
}

--index.js--

process.stdin.resume();

process.on('SIGINT', () => {
  console.log('\n');
  console.log('Doing async cleaning');
  
  setTimeout(() => {
    console.log('Done');
    process.exit(0);
  }, 500);
});
@heisian

This comment has been minimized.

Copy link

commented Nov 29, 2018

Agreed, this makes it impossible to use yarn for running processes that manage child processes.

@heisian

This comment has been minimized.

Copy link

commented Nov 29, 2018

Here is what I ended up doing:

// The following captures keypress events and effectively blocks any of the
// CTRL+C/CTRL+Z/CTRL+D combinations from sending their respective signals.

readline.emitKeypressEvents(process.stdin)

if (process.stdin.isTTY) process.stdin.setRawMode(true)

process.stdin.on('keypress', (char, key) => {
  if (key.ctrl && key.name === 'c') {
    console.log("Caught CTRL+C and asking you to wait for this process to exit gracefully, mi'lord.")
    const exitCode = 0
    cleanupHandler(exitCode)
  }
})
@fyodorvi

This comment has been minimized.

Copy link

commented Apr 5, 2019

Same here, any ideas if this can be fixed?

@bodinsamuel

This comment has been minimized.

Copy link

commented Sep 12, 2019

Issue is still valid, is there any progress? ☺️

This can be reproduced with a very simple setup script

const wait = waitTime => new Promise(resolve => setTimeout(resolve, waitTime));
setInterval(() => {
  console.log("interval");
}, 1000);

process.on('SIGINT', async () => {
  console.log('caught');
  await wait(1000);
  console.log('killing');
  process.exit(0);
});

when doing node test.js it waits for proper killing, but not with yarn node test.js

Screenshot 2019-09-12 12 08 21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.