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

Can't use bash history after SIGINT with Yarn #1647

Closed
waynebloss opened this issue Dec 9, 2019 · 18 comments
Closed

Can't use bash history after SIGINT with Yarn #1647

waynebloss opened this issue Dec 9, 2019 · 18 comments
Labels
specific to environment stale no activity for 2 weeks

Comments

@waynebloss
Copy link

  • nodemon -v: 2.0.1
  • node -v: v12.13.0
  • Operating system/terminal environment: Linux 4.19.85-1-MANJARO x86_64
  • Using Docker? What image: No
  • Command you ran: yarn start

...with package.json configured as:

{
  "scripts": { "start": "nodemon" },
  "nodemonConfig": {
    "watch": ["src"],
    "ext": "ts",
    "ignore": [],
    "exec": "ts-node ./src --env=development"
  },
  "dependencies": {
    "express": "^4.17.1"
  },
  "devDependencies": {
    "@types/express": "^4.17.2",
    "ts-node": "^8.5.4",
    "typescript": "^3.7.3"
  }
}

Expected behaviour

After using CTRL+C to stop nodemon, I should be able to use my up and down keyboard arrows to navigate bash history.

Actual behaviour

Instead, after using CTRL+C and pressing the up arrow key, my console prints ^[[A. (If I hit the down arrow key, it prints ^[[B.) After I press CTRL+C a second time, I am able to navigate bash history.

However, if I don't press CTRL+C a second time I am still able to manually type the command yarn start to start again.

Also, the first time CTRL+C is used, my shell prints out ^C once and produces a new prompt (where up/down doesn't work). Then the second time I press CTRL+C to fully exit, my shell prints out ^C^C instead of just one ^C.

Steps to reproduce

  1. Use yarn to run nodemon with the configuration presented above.

More information


My program

import express from "express";

const app = express();
const port = 4081;
app.get("/", (req, res) => {
  res.send("Hello from TypeScript!");
});
app.listen(port, err => {
  if (err) {
    return console.error(err);
  }
  return console.log(`server is listening on http://localhost:${port}`);
});
@waynebloss
Copy link
Author

If this is an issue for Yarn, I will gladly file it with them. Thank you.

@remy
Copy link
Owner

remy commented Dec 11, 2019

I can't replicate this (yet - I'm on a mac, and this sounds specific to your OS), but you can try with nodemon@2.0.2 - this changed the way the sub-process is spawned.

@stephensli
Copy link

I was able to re-create this (windows - wsl) and this is what I was able to determine in the time I had.

This issue is triggering the quit path. run.js is emitting the quit event with code 130. This is resulting in the bus.on('quit') event being fired and the kill(child, 'SIGINT') being called, calling into the exit method which does funnel down to the process.exist(code).

With yarn, the terminal acts as if the standard in as been released but has not completed executing code. Adding in logs (could not get the debug logs to run at the time) can show this clearly in the console (as shown below). This does not occur within npm and just executes all and exists.

image

I did not have much time to investigate but hope this helps.

@waynebloss
Copy link
Author

Confirmed Linux only. Could not reproduce on Mac in the default terminal app or on Windows 10 with Git Bash, Powershell or CMD.exe.

On Manjaro Linux I can reproduce with Xfce Terminal and Tilix.

@waynebloss
Copy link
Author

I meant to say - I tried with nodemon@2.0.2 as well and there was no change.

@stale
Copy link

stale bot commented Dec 25, 2019

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

@stale stale bot added the stale no activity for 2 weeks label Dec 25, 2019
@stale stale bot closed this as completed Jan 1, 2020
@adamreisnz
Copy link

I can reproduce this in OS X 10.15.3 with zsh instead of bash. Exact same symptoms. Pressing CTRL-C kills the app, but the zsh subshell keeps running, requiring a 2nd CTRL-C to properly exit. This is with nodemon ^2.0.4.

@adamreisnz
Copy link

@remy can this be reopened please?

@remy
Copy link
Owner

remy commented Nov 4, 2020

@adamreisnz do you have a repo or gist that can be used to replicate the issue?

@adamreisnz
Copy link

I will try to put it together

@adamreisnz
Copy link

Ok, I have managed to make this example simple which reproduces the problem.

Note that it doesn't happen 100% of the time with this example, but often enough to be noticeable after you start/stop the script several times.

package.json

{
  "name": "test",
  "description": "Test",
  "version": "1.0.0",
  "homepage": "https://helloclub.com",
  "author": {
    "name": "Hello Club Ltd",
    "email": "info@helloclub.com",
    "url": "https://helloclub.com/"
  },
  "main": "index.js",
  "scripts": {
    "start": "nodemon index.js"
  },
  "dependencies": {
  },
  "devDependencies": {
    "nodemon": "^2.0.4"
  }
}

index.js

const http = require('http');
const server = http.Server();

//Listen on port
server.listen(8080, function() {
  console.log('Server running')
});

Simply start the script with yarn start and then once the server is running, stop the script with CTRL-C and press up to verify that zsh has fully exited or not:

image

@waynebloss
Copy link
Author

@remy I'd still like to get this resolved if possible. Can you say what needs to be done to help you reproduce it?

There is a reproduction above from another Mac user. I also have a Mac to try it on. My bigger concern is with my main workstation though because I only use my Mac for iOS stuff.

On Linux I can reproduce it every time. When I use create-react-app there the issue doesn't occur so there's definitely something going on between nodemon and yarn because create-react-app does the same watch/restart functionality and that runs with yarn start as well.

What version of Mac OS are you running? I have Mojave on mine. Should I just put @adamreisnz reproduction in a repo or is there something else I can do?

@remy
Copy link
Owner

remy commented Mar 27, 2021

Basically it boils down to if I can't replicate it myself then there's no way for me personally to resolve the issue.

There's the code by @adamreisnz that shows that they can replicate, but this doesn't work for me at al - my history is intact…though I notice that you specify bash and @adamreisnz's example is zsh (which I also have):

Screenshot 2021-03-27 at 14 59 08

So if you can replicate, then you'll need to take point on the PR - fork the repo, I suggest then doing npm link on nodemon so you're using your local copy and start looking at lib/run.js which is where nearly all the exit handling is done.

I'd probably personally take two pronged approach:

  1. Try removing all the exit logic and see if the history issue is resolved - and if so, start adding parts back until you're able to narrow it down.
  2. Possibly creating a super pared down process that replicates what nodemon is doing from a 'spawn a sub process and then end it' point of view - to see if somehow that's the problem, though it's super vague

Oh, one thing you could try to see if it's related is using nodemon with the --spawn option. This means instead of forking the sub-process it'll spawn a new shell. If somehow nodemon is messing with your session (or nodemon via yarn perhaps?).

The thing that's actually the big read flag for me is when you said:

This problem does not happen if I use npm run start

This means it's directly related to how yarn is spawning nodemon - which is … specific to yarn and I'm not familiar with yarn's code and spawning process at all.

@waynebloss
Copy link
Author

@remy Sorry for wasting your time! After searching around for other issues surrounding Yarn and SIGINT I found a very good description of what's happening here

If yarn or npm get a SIGINT or SIGTERM signal, they correctly forward the signal to spawned child process (in this case node index.js). However, it does not wait for the child processes to stop. Instead, yarn/npm immediately stop themselves. This signals to Docker/Kubernetes that the container is finished and Docker/Kubernetes will kill all children processes immediately. There are issues open for Yarn and NPM but unfortunately they are not solved yet.

This led me to yarnpkg/yarn#4667 which actually mentioned nodemon and was already linked to from another nodemon issue (1326, which I linked to here!)

The workaround mentioned there seems very heavy handed, so I would say that's a no-go.

The problem is basically that any child process of yarn start that waits or sets timeouts in a SIGINT handler will cause the issue.

I should have seen this right when I filed the original issue. I'm just surprised it's been going on this long because many other projects seem to have been affected by this.

@waynebloss
Copy link
Author

waynebloss commented Mar 27, 2021

@remy I did one experiment in my project to see if normal exiting without Ctrl+C is affected. Here's what I did:

main(); // Starts an Express server...

setTimeout(() => {
  process.exit();
}, 3000);

With this code in place, I get:

Server started.

[nodemon] clean exit - waiting for changes before restart
^C

EDIT: (And things exited successfully, bash history was working after this.)

So, the one workaround that nodemon could offer is a q command (like rs but to quit instead of restart). EDIT: Because yarn would never receive a SIGINT and exit too early...

When I used nodemon as a library a long time ago, I actually implemented this myself and shared it in another issue that you helped me with.

@waynebloss
Copy link
Author

waynebloss commented Mar 27, 2021

If you don't want to offer a q command for some philosophical reason - no problem, I won't do the work. If you would like me to do the work though, I'm pretty good at following project rules and styles and I'd be glad to make a pull request for it. Thanks!

@remy
Copy link
Owner

remy commented Mar 28, 2021

Just trying to get my head around the issue - am I right I the following:

  • user sends ctrl+c
  • yarn also sees this and starts a short lived timeout
  • nodemon tries to exit cleanly (waiting for sub process to end)
  • yarn exits early and in doing so also breaks bash history

I'm not so keen on adding a new/different way to end the process - I prefer UX that requires no learning.

Would it be possible to:

  • detect that nodemon was spawned inside of yarn
  • if true, then trap sigterm signals and do not let them bubble up
  • then handle closing of sub process and then exit 130 (or 1 since it's an interrupt)

Would that work? I'm sure that yarn has some extra environment values that could be detected.

@waynebloss
Copy link
Author

waynebloss commented Mar 29, 2021

@remy Your issue explanation matched my reading of things from the sources I referenced. But, after looking some more I think I can see that they don't even send a timeout. They just don't handle it at all.

I'm not sure if you can trap the signal. Yarn is responsible for forwarding them based on what I've read, but apparently it doesn't do that at all.

That 3-year old comment says that " it doesn't look like SIGINT is handled at all [by yarn]" outside of running in mutex mode (an uncommon mode for use on servers), where a packaged called death is used in 2 places.

Deceivingly, it now looks like they are catching SIGINT it in the latest stable code right in the beginning start command handler. BUT this is only executed if there is a yarn-path defined in a yarnrc file. A yarn-path "Instructs yarn to defer to another Yarn binary for execution." so they're only doing it for their "yarn v2" binary which is not the current stable default, you have to opt-in to it since it breaks a lot of things. Interesting...

 if (rc.yarnPath is defined in an rc file) {
   // In the yarn start command. However, only called in the special condition above...
   process.on(`SIGINT`, () => {
      // We don't want SIGINT to kill our process; we want it to kill the
      // innermost process, whose end will cause our own to exit.
    });
  }

In v2 they are now propogating SIGTERM to spawned child processes (SIGINT and SIGTERM if you look at the files changed in the merge request).

In light of this, I wonder if you could detect that nodemon was spawned inside of yarn and do the wrong thing (just kill the child processes and exit without waiting), maybe optionally.

Other than that, here's one thing I tried naively, which didn't work:

{  "scripts": { "start": "trap 'exit 0' SIGINT; nodemon" },
  "nodemonConfig": {
    "watch": ["src/"],
    "ext": "js,json,ts",
    "exec": "babel-node -x \".ts\",\".js\" src/index.ts"
  } }

With the above in my package.json file, I run the cli command: yarn start. It didn't fix the issue - I wasn't sure what to expect.

Looking again now I can see that the yarn cli itself is implemented using an old v2.9 of a package called commander by tj, which I'm not sure how it works - I stopped there. I do see it handling signals = ['SIGUSR1', 'SIGUSR2', 'SIGTERM', 'SIGINT', 'SIGHUP'] and calling proc.kill on sub commands, but I didn't follow the logic all the way in from yarn to make sure it's actually executing that code. Looks like it doesn't wait and just exits though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specific to environment stale no activity for 2 weeks
Projects
None yet
Development

No branches or pull requests

4 participants