open-cli-tools / concurrently

Run commands concurrently. Like `npm run watch-js & npm run watch-less` but better.
https://www.npmjs.com/package/concurrently
MIT License
6.99k stars 227 forks source link

SIGINT is sent twice when pressing Ctrl-C, causing dirty shutdown #283

Open alimony opened 3 years ago

alimony commented 3 years ago

I have a package.json with the following scripts:

…
"startemulators": "firebase emulators:start --import seed",
"listen": "onchange 'source_files/*.js' -- touch functions/index.js",
"emulate": "concurrently \"npm run startemulators\" \"npm run listen\""
…

Running the emulate script works as expected, but pressing Ctrl-C to shut the processes down gives me this:

[0] i  emulators: Received SIGINT (Ctrl-C) for the first time. Starting a clean shutdown.
[0] i  emulators: Please wait for a clean shutdown or send the SIGINT (Ctrl-C) signal again to stop right now.
[0] i  emulators: Shutting down emulators.
[0] i  ui: Stopping Emulator UI
[0] ⚠  Emulator UI has exited upon receiving signal: SIGINT
[0] i  functions: Stopping Functions Emulator
[0] i  hosting: Stopping Hosting Emulator
[0] i  database: Stopping Database Emulator
[1] npm run listen exited with code 0
[0]  
[0] ⚠  emulators: Received SIGINT (Ctrl-C) 2 times. You have forced the Emulator Suite to exit without waiting for 3 subprocesses to finish.

So for some reason, a second SIGINT is sent to the firebase command specified in the first script. How come?

alimony commented 3 years ago

Any ideas on this?

akauppi commented 3 years ago

@alimony I have seen this, but wasn't considering concurrently would be causing the double Ctrl-C's.

It may well be. In that case, debugging it means having a project with Firebase CLI and concurrently both in the mix. Maybe we should create a minimal project that showcases this?


I can reproduce it:

⚠  emulators: Received SIGINT (Ctrl-C) 2 times. You have forced the Emulator Suite to exit without waiting for 1 subprocess to finish. These processes may still be running on your machine: 

┌────────────────────┬──────────────┬─────┐
│ Emulator           │ Host:Port    │ PID │
├────────────────────┼──────────────┼─────┤
│ Firestore Emulator │ 0.0.0.0:6767 │ 32  │
└────────────────────┴──────────────┴─────┘

macOS 11.4, firebase-tools 9.12.1, concurrently 6.2.0

alimony commented 3 years ago

I'll create a minimal test case.

akauppi commented 3 years ago

I can provide a reproducible, negative test case.

$ git clone https://github.com/akauppi/firebase-jest-testing.git
$ cd firebase-jest-testing
$ git checkout next     # ADDED THIS LATER!
$ npm install
$ npm run //start:pipeless
...
<once services are up, press Ctrl-C>

Tried ~6 times, not a single time there is Received SIGINT (Ctrl-C) 2 times.


I'll try to compare with my other project, where I do see these, and try to spot the difference.

The weird "//start:pipeless" is because if I pipe Firebase Emulator output, another concurrently issue is raised, and it disturbs in checking this one. :)

macOS 11.4

akauppi commented 3 years ago

The positive case comes here.

If I launch: concurrently -> Docker -> Firebase CLI (which I do in a web app project, all the time), the double Ctrl-C occurs.

Detailed log ``` > concurrently --kill-others "$(docker-run-cmd) firebase emulators:start --project=demo-2" Launching Docker... 🐳 [0] ⚠ emulators: You are not currently authenticated so some features may not work correctly. Please run firebase login to authenticate the CLI. [0] i emulators: Starting emulators: auth, functions, firestore [0] i emulators: Detected demo project ID "demo-2", emulated services will use a demo configuration and attempts to access non-emulated services for this project will fail. [0] ⚠ Your requested "node" version "14 || ^16" doesn't match your global version "16" [0] ⚠ functions: You are not signed in to the Firebase CLI. If you have authorized this machine using gcloud application-default credentials those may be discovered and used to access production services. [0] i firestore: Firestore Emulator logging to firestore-debug.log [0] i ui: Emulator UI logging to ui-debug.log [0] i functions: Watching "/work/functions" for Cloud Functions... [0] ✔ functions[us-central1-cloudLoggingProxy_v0]: http function initialized (http://0.0.0.0:5002/demo-2/us-central1/cloudLoggingProxy_v0). [0] ✔ functions[us-central1-userInfoShadow_2]: firestore function initialized. [0] [0] ┌─────────────────────────────────────────────────────────────┐ [0] │ ✔ All emulators ready! It is now safe to connect your app. │ [0] │ i View Emulator UI at http://0.0.0.0:4000 │ [0] └─────────────────────────────────────────────────────────────┘ [0] [0] ┌────────────────┬──────────────┬───────────────────────────────┐ [0] │ Emulator │ Host:Port │ View in Emulator UI │ [0] ├────────────────┼──────────────┼───────────────────────────────┤ [0] │ Authentication │ 0.0.0.0:9100 │ http://0.0.0.0:4000/auth │ [0] ├────────────────┼──────────────┼───────────────────────────────┤ [0] │ Functions │ 0.0.0.0:5002 │ http://0.0.0.0:4000/functions │ [0] ├────────────────┼──────────────┼───────────────────────────────┤ [0] │ Firestore │ 0.0.0.0:6767 │ http://0.0.0.0:4000/firestore │ [0] └────────────────┴──────────────┴───────────────────────────────┘ [0] Emulator Hub running at localhost:4400 [0] Other reserved ports: 4500 [0] [0] Issues? Report them at https://github.com/firebase/firebase-tools/issues and attach the *-debug.log files. [0] ^C[0] [0] i emulators: Received SIGINT (Ctrl-C) for the first time. Starting a clean shutdown. [0] i emulators: Please wait for a clean shutdown or send the SIGINT (Ctrl-C) signal again to stop right now. [0] i emulators: Shutting down emulators. [0] i ui: Stopping Emulator UI [0] ⚠ Emulator UI has exited upon receiving signal: SIGINT [0] i functions: Stopping Functions Emulator [0] i firestore: Stopping Firestore Emulator [0] [0] ⚠ emulators: Received SIGINT (Ctrl-C) 2 times. You have forced the Emulator Suite to exit without waiting for 1 subprocess to finish. These processes may still be running on your machine: [0] [0] ┌────────────────────┬──────────────┬─────┐ [0] │ Emulator │ Host:Port │ PID │ [0] ├────────────────────┼──────────────┼─────┤ [0] │ Firestore Emulator │ 0.0.0.0:6767 │ 30 │ [0] └────────────────────┴──────────────┴─────┘ [0] [0] To force them to exit run: [0] [0] kill 30 [0] [0] docker run --rm --sig-proxy=true -v /Users/asko/Git/GroundLevel-firebase-es/packages/backend:/work -w /work -p 6767:6767 -p 5002:5002 -p 9100:9100 -p 4000:4000 firebase-ci-builder:9.12.1-node16-npm7 firebase emulators:start --project=demo-2 exited with code 0 ```

Note: This is just my 2c. I am not really troubled by this error message, since if running things under Docker, things get cleaned up, anyhow.

alimony commented 3 years ago

I have not been able to create a small test case that consistently shows this error, but I have discovered that skipping the pubsub emulator makes the error go away. For example, using this:

firebase emulators:start --only firestore,database,functions,hosting,pubsub

will send SIGINT twice when quitting concurrently, while this:

firebase emulators:start --only firestore,database,functions,hosting

will not.

ghivert commented 3 years ago

Hi,

I have the same problem and investigated a little bit. By commenting out that line, I ended up with something working correctly, no double SIGINT, everything is cool.

It looks like spawn-command does not spawn the subprocess in detached mode, and it seems the defaults in concurrently are not setting detached options neither. Because if not, it's perfectly normal to have the SIGINT sent twice, because it's received by the subprocess, and then resent with command.kill(signal) a second time.

idudinov commented 3 years ago

hi there! I'm experiencing this issue too if I try to run firebase directly as concurrently command.

However, if my firebase command is wrapped with yarn script command in package.json, the issue is not reproduced. Looks like yarn already does some protection of double sent signals.

I can't use full command in scripts since I construct firebase command args on the fly. So I did an alias like that:

// package.json

"scripts": {
  "firebase:emulators": "firebase emulators:start",
}

and then call the command via concurrently like that:

yarn firebase:emulators --only ... <here are my dynamic args>

for me this is a legit workaround, hope it helps someone else

samtstern commented 2 years ago

Running into this problem myself, here's a bit from my package.json:

  "scripts": {
    "build": "tsc",
    "build:watch": "tsc -w",
    "format": "prettier --write src/**/*.ts",
    "emulate": "firebase emulators:start --import=./emulator-data --export-on-exit",
    "serve": "concurrently 'npm run build:watch' 'npm run emulate'"
  },

If I run npm run serve and then press Ctrl+C the Firebase emulators see double SIGINT signals and shutdown without exporting data.

gustavohenke commented 2 years ago

Hey all. I've briefly looked at this today, it seems that firebase-tools@9 causes the issue, but firebase-tools@10 doesn't.

@akauppi it seems you did a fair amount of research in https://github.com/firebase/firebase-tools/issues/3578. Is there much that should be done here?

I see people mentioned the removal of things like passing SIGINT down to the command or spawning subprocesses in detached mode, but I'm afraid this would worsen issues around hanging processes.

akauppi commented 2 years ago

@gustavohenke I moved from concurrently to Docker Containers last year, and that removed the dirty shutdown issue for me.

jakeleventhal commented 2 years ago

Same issue with firebase export

jakeleventhal commented 2 years ago

A solution i came up with is like this:

{
    "emulator-start": "./node_modules/.bin/firebase emulators:start --import=emulatorData/localEmulatorData --export-on-exit=emulatorData/localEmulatorData",
    "start": "npm run emulator-start & npm run start-servers",
    "start-servers": "concurrently -c gray.dim --kill-others \"npm run api\" \"npm run scheduler\""
}
jasonkylefrank commented 2 years ago

@jakeleventhal So you just replaced concurrently with the & approach for the npm script which combines the emulators with another command?

My understanding is that the & approach will not work on Windows machines. Is that still true?(I'm on Mac and don't have a Windows machine handy to test it with).

jakeleventhal commented 2 years ago

@jasonkylefrank kind of - i am running the emulators outside of concurrently but my servers are still running via concurrently

as for windows - i don't use windows so idk lol

hixus commented 2 years ago

I noticed this today as now I can't exit the shell with pressing ctrl+c again (on macbook pro).

running "concurrently \"yarn:firebase:emulators\"" and pressing ctrl+c gives exit code 1. Running the same without concurrently gives exit code 0.

Adding docker to mix with "concurrently \"yarn:firebase:emulators\" \"docker compose up\" and pressing ctrl+c I get

^C[1] Gracefully stopping... (press Ctrl+C again to force)
[firebase:emulators]
[firebase:emulators] i  emulators: Received SIGINT (Ctrl-C) for the first time. Starting a clean shutdown.
[firebase:emulators] i  emulators: Please wait for a clean shutdown or send the SIGINT (Ctrl-C) signal again to stop right now.
[firebase:emulators] i  emulators: Shutting down emulators.
[firebase:emulators] i  ui: Stopping Emulator UI
[firebase:emulators] ⚠  Emulator UI has exited upon receiving signal: SIGINT
[firebase:emulators] i  auth: Stopping Authentication Emulator
[firebase:emulators] i  hub: Stopping emulator hub
[1] Container gql-1  Stopping
[firebase:emulators] i  logging: Stopping Logging Emulator
[1] time="2022-05-03T08:09:27+03:00" level=error msg="got 3 SIGTERM/SIGINTs, forcing shutdown"
[1] Container db-1  Killing
[1] Container gql-1  Killing
[firebase:emulators] yarn run firebase:emulators exited with code 1
[1] Container db-1  Killed
[1] Container gql-1  Killed
[1] Container gql-1  Stopped
[1] Container db-1  Stopping
[1] Container db-1  Stopped

concurrently process seems to be stuck itself so I cant exit the shell anymore

LaysDragon commented 2 years ago

@gustavohenke I see people mentioned the removal of things like passing SIGINT down to the command or spawning subprocesses in detached mode, but I'm afraid this would worsen issues around hanging processes.

  • windows 10
  • firebase-tool 11.2.2
  • concurrently@7.2.2

I'm also have this problem that double SIGINT cause emulator unable to export on exit gracefully. I remove and patched the line that @ghivert mentioned,and its really solve the problem on windows. Double SIGINT seems really not a right way to exit all process, maybe consider use the timeout timer to deal with hanging process in any case?

Zehir commented 1 year ago

Same issue over here, any updates ?

jahudka commented 1 year ago

I think this may be due to Concurrently's use of tree-kill to terminate individual processes. For example, if you send a SIGINT signal to a Docker Compose, it will handle terminating its subprocesses on its own; there's no need for Concurrently to know or care that the process has subprocesses (although I'm sure that's not always the case). But I think having the option to override this behaviour with simply sending the signal to each of the original processes might solve this issue.

I wanted to verify this by writing a minimal wrapper for the Concurrently API and passing (pid, signal) => process.kill(pid, signal) as the kill option; but it doesn't quite seem to work as it should - there's no output at all (even when I pass outputStream: process.stdout in the options), and the script seems to terminate immediately when I hit Ctrl+C, as opposed to waiting for the child processes to exit, like the CLI does; so I don't really know how to verify this.