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

fix(gatsby): Release stdio streams when exiting `gatsby develop` #16714

Merged
merged 26 commits into from Aug 28, 2019

Conversation

@Js-Brecht
Copy link
Contributor

commented Aug 18, 2019

Description

When using the bash shell, stdio streams are not returned to the shell when gatsby develop exits after receiving a SIGINT signal.

I believe this is due to this node behavior:

https://nodejs.org/api/process.html#process_signal_events:
SIGTERM and SIGINT have default handlers on non-Windows platforms that reset the terminal mode before exiting with code 128 + signal number. If one of these signals has a listener installed, its default behavior will be removed (Node.js will no longer exit).

Fairly simple fix: Exit the process gracefully by releasing stdio streams from readline interface prior to calling process.exit(0) (I added the exit code, since exit was initiated by the user).

One alternative would be to pass the signal on to the node process:

process.kill(process.pid, `SIGINT`);

However, I was concerned about Windows compatibility, since Windows does not support signals, as such. I believe that would require additional logic to account for a Windows environment, which would wind up being a simple process.exit(0) anyway....

Node documentation isn't clear about what "reset the terminal mode" means in its entirety. It's possible that if you have custom controls configured in the terminal, they might be overwritten by Node, and exiting like this will not reset them. In that case, relaying the signal to node will (I believe) cause node to execute its default behavior.

Related Issues

Fixes #16621

Jeremy Albright added 2 commits Aug 17, 2019
@Js-Brecht Js-Brecht requested a review from gatsbyjs/core as a code owner Aug 18, 2019
@lannonbr lannonbr added the topic: cli label Aug 18, 2019
@rjray

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

I applied this patch directly to my local copy in node_modules/gatsby/dist/commands/develop.js, and it has not affected the behavior after Ctrl-C at all. This is still on Gatsby 2.13.42 (gatsby-cli 2.7.21), so there might be other newer changes that affect this.

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@rjray Can you try this snippet?

rlInterface.on(`close`, () => {
  // fixes issue with bash shell not receiving control of
  // stdio when exiting
  rlInterface.close() // Release stdio streams
  process.kill(process.pid, `SIGINT`) // Relay signal to node
  process.kill(process.ppid, `SIGINT`) // Relay signal to parent process (/bin/sh)
})

I know where I went wrong in my testing. My testing was done with npx, and this PR fixes it when used like that. My guess is that by releasing the stdio streams, then exiting, npx is able to perform the rest of the clean up.

When running simply gatsby develop, it does still fail. 😖

I tested the above code using gatsby develop, instead of npx gatsby develop, and it began working correctly. I need to do some testing on Windows, though, to make sure there's no unintended side effects.

@rjray

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@Js-Brecht Applied the patch and tested (Ubuntu 16.04), it works.

Well done 🥇!

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Glad to hear it. Assuming all goes well, I will update the PR shortly

stdio was still hanging when running `gatsby develop` in a bare shell.  This resolves the issue.  Fixes additional issues with the CLI colors not being reset on Windows, as well.
@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

I'm happy to report that this works great on Windows. There were even some issues I had noticed from time to time, where the CLI colors were wrong after exiting (in Powershell). This seems to resolve that issue, as well. 🚀

@wardpeet wardpeet self-assigned this Aug 19, 2019
@wardpeet

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Awesome job! I wonder if we could do it without the process.kill by moving some code around.

We remove the rlInterface.on('close', ...) and close it after we detected the port on

program.port = await detectPortInUseAndPrompt(port, rlInterface)

  const rlInterface = rl.createInterface({
    input: process.stdin,
    output: process.stdout,
    terminal: false, // @see https://github.com/nodejs/node/issues/21771
  });
  program.port = await detectPortInUseAndPrompt(port, rlInterface); // Start bootstrap process.
  rlInterface.close();

Unsure if this makes sense, no need to keep rlInterface running.

@wardpeet wardpeet changed the title Release stdio streams when exiting `gatsby develop` fix(gatsby): Release stdio streams when exiting `gatsby develop` Aug 19, 2019
@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@wardpeet Yes, that should work, I like it 🙂.

I think if we also checked for the port prior to retrieving the ssl certificate, I'm pretty sure that would fix an issue I failed to mention with #16726 causing a double echo when asking for a password.

Will implement later today, and we can see how it does.

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

So, I went on an adventure through Windows land last night and encountered some interesting behavior with readline.

First thing, closing the interface outside of a closing process causes the console to hang and crash when you hit CTRL+C...

So this:

const rlInterface = rl.createInterface({
    input: process.stdin,
    output: process.stdout,
    terminal: false, // @see https://github.com/nodejs/node/issues/21771
  });
  program.port = await detectPortInUseAndPrompt(port, rlInterface); // Start bootstrap process.
  rlInterface.close();

Would cause a crash. I thought that maybe it was that since the streams had been closed, the signal was reaching the console process. If I remember correctly, listening on the close signal fixed the problem, but you wind up with the same code as before, passing SIGINT up the chain.


ALSO, changing to that doesn't fix the double echo caused by devcert. The issue occurs because readline is colliding with password-prompt. password-prompt is reading directly from stdin, processing, then echoing the character you entered back to the CLI, which is causing the double echo. There's more issues, too, such as the input not being masked like it's supposed to be, but that's an issue for another time.

So, I spent some time troubleshooting, and trying to isolate the issue. Apparently, a readline.resume() will fix that issue. What was interesting, and this is where it started to get weird, you can put that resume() function anywhere. I went so far as to isolate the readline interface, so that it was only ever required and instantiated when it was needed.

It looked like this: detect-port-in-use-and-prompt.js

const readlinePort = (port) => {
  const question = `Something is already running at port ${port} \nWould you like to run the app at another port instead? [Y/n] `

  const rlInterface = require(`readline`).createInterface({
    input: process.stdin,
    output: process.stdout,
    terminal: false,
  })
  
  return new Promise(resolve => {
    rlInterface.question(question, answer => {
      // Putting this HERE fixes the double echo, too ?!?
      rlInterface.resume()
      // This causes the terminal to terminate when receiving ^C
      // EVEN IF this code is never executed!!
      // rlInterface.close()
      resolve(answer.length === 0 || answer.match(/^yes|y$/i))
    })
  })
}

Even when that branch of code is NEVER executed, that resume() function still works to fix the double echo. What's crazy, is even if I put the close() function there, where it NEVER gets executed if the port is not in use, it will STILL cause the console to crash when you hit CTRL+C! And it doesn't matter if I put program.port = await detectPortInUseAndPrompt(port, rlInterface); before getSslCert, or after! Makes no difference whatsoever.

The only thing I can think is that, since readline is a node interface, node is doing some kind of optimization when it's compiling the code. It's seeing it, and processing it during the JIT stage. So it's trapping the streams with readline, no matter where you initialize the interface... and then resuming/closing them.... before it uses them???

What is also strange is that the behavior of readline doesn't really fit what the documentation says.

  1. The rl.resume() method resumes the input stream if it has been paused.

  2. The rl.pause() method pauses the input stream, allowing it to be resumed later if necessary.

  3. The rl.close() method closes the readline.Interface instance and relinquishes control over the input and output streams. When called, the 'close' event will be emitted.

readline is never paused.. why would resume work? What else is it doing that is undocumented. Also, will readline cause a leak if it is not closed? Does it even matter? If it is going to be processed no matter where you put it, might as well just put it in the only place it's going to be needed, instead of passing it down through several layers of functions that don't even care about it.

I want to do some more testing later. I still have some questions, and I want to get the testing parameters fixed a little better in my head.

@wardpeet On a side note: I do think that thedetectPortInUseAndPrompt() check should be executed before fetching the ssl certificates. Getting the ssl certificates is a much more expensive process, and since it produces side-effects beyond the scope of the runtime by altering your filesystem/certificate store, it shouldn't even run if you aren't even going to start the server (potentially, because the port is in use). Just my opinion.

@wardpeet

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@Js-Brecht Awesome write up, thanks for looking into this. There is not much value I can add to your comments than drown in my own tears 😭.

This is how enquirer solves it https://github.com/enquirer/enquirer/blob/9eef2ed3b4b6222985e4eb85098d3c5a3b2b8b93/lib/keypress.js#L195-L220.

I'm fine with moving ssl generation down if that fixes our problem ;) I don't think there is a particular reason why it's above port check.

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

It was making me want to tear my hair out 🤣.

Thanks for the link. I had been looking at enquirer, because they have a password prompt, and wanted to see how they handled it. But I hadn't seen that part yet; I was looking at a deprecated module, as it turns out.

I see they issue a readline.resume() during initialization, so... if it works, it works. Let me see what I can put together that will build the interface, and tear it down a little more elegantly, without needing all the interrupt/kill signals. Should have something soon'ish. I've got other projects that really need to be taken care of before this week is out.

Jeremy Albright and others added 7 commits Aug 23, 2019
`readline-interface` module for `readline` stream management abstraction
@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@wardpeet Please review my latest update, if you would. More detail can be found @ Js-Brecht/gatsby-1#2.

Wound up being more work than I anticipated. readline is not a fun one to work with 😆. Because of the added complexity behind getting this to work, I decided to just create a new module that would abstract away most of the details.

I've tested this with Windows powershell, git bash, mintty (that one was ridiculous 🙄), wsl, and on Linux using bash, and zsh. It performs well.

If you see anything that doesn't look right, please let me know.

I also tested this using the devcert upgrade, and it fixes the double-echo. That was a trick.

This module can even be extended to mask input fairly easily, I would think, and it's not as heavy-weight as enquirer. Could be an alternative to what devcert uses to generate the password prompt. This module shouldn't cause any collisions, either. I could extract it into its own npm package, then try to get them to use it over there.

I'm pretty confident that it'll fix other issues I noticed with their password prompt.

Of course, an alternative would be to just use enquirer...

@wardpeet

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

@Js-Brecht OMG! 🎉 Thanks so much for your hard work and struggles. I feel a bit bad about saying this after all the sweat and tears you've put in this. I believe we probably best include "prompts" as a package instead. We use prompts in gatsby-cli which is a dependency of gatsby so we could also use it here without introducing a new package.

Why prompts over your awesome custom solution? We like using well-tested packages which are actively maintained and are globally used. This gives us the confidence that we can rely on and ask questions about people who have more experience in certain areas.

I don't mind driving this PR over the finish line if you're a bit occupied with other stuff. I would love to get this fix in as current behaviour is annoying.

Awesome work! I wouldn't have been able to fix this the way you did! 👏 👏 You're a hell of an engineer.

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Ohh, prompts! If I had known about that one, I would have just gone that route in the first place 😆. Would have saved a headache or two. Still familiarizing myself with Gatsby, though; of course I should have thought to look in gatsby-cli 🙄.

This module will just go into my personal collection. Probably rewrite it using Typescript and promises. I'll find a use for it somewhere; I write CLIs for various things all the time.

I don't feel bad at all setting this up to use prompts instead. It makes perfect sense to use what's already been battle-tested, and is already being used across the repository. I'll do that now; shouldn't take long, I don't think.

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

There we go. Much more succinct. Works like a charm, from where I'm at; I'm only able to test in Windows (powershell, mintty, wsl, bash) for right now, though. I don't have access to my linux box from here. Can somebody try run it using bash in Linux/MacOS? Pretty certain it'll work.

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@Js-Brecht wow thanks for all the hard work investigating and testing solutions! It'll be great to get this fixed for everyone! 🙏

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

More than happy to do it 🙂

Copy link
Member

left a comment

I've done a small change to serve & to error handling. Thanks for all your hard work! I'll merge when it's green.

I can't say thank you enough for your investigation and fixes.

@gatsbybot gatsbybot merged commit 280cf7f into gatsbyjs:master Aug 28, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Congrats.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:30
Details
unit_tests_windows Build #20190828.59 succeeded
Details
@wardpeet

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Published in gatsby@2.14.2

waltercruz added a commit to waltercruz/gatsby that referenced this pull request Sep 8, 2019
…sbyjs#16714)

* Cleanup readline stdio streams

* documentation of issue

* Resolves issue with stdio still hanging on bash

stdio was still hanging when running `gatsby develop` in a bare shell.  This resolves the issue.  Fixes additional issues with the CLI colors not being reset on Windows, as well.

* readline module

* readline prompt

* Using `mute-stream`

* Using `readline-interface.prompt`

* Utility module wrapping readline, providing streams/prompt control

* doesn't need to be async; using callbacks...

* exposed by prompt interface, albeit with a preset readline interface

* Not using `readline-sync`.  That was a test

* [interim] working on mintty bug with setRawMode()

* Single character input

* Additional documentation/typing

* [bug-fix] Fixed mintty issue with `setRaw`

* Forgot to remove (unneeded) argument

* Event listener caching, to guarantee proper listener management

* documentation

* Type updates

* [bug-fix] running ask() function multiple times would cause duplicate listeners

* [bug-fix] fixes continued double echo when other processes write to stdio

* Use `prompts` package instead of `readline-interface`

* fix serve & throw instead of promise.reject
@sidharthachatterjee

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Such ridiculously good work here! Featuring on this month's Gazette in #17548

@ehowey

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

@Js-Brecht Any chance your changes would be related to this issue: #17131

The timing of when I started experiencing this error matches up about right...

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

@ehowey Well, the only place this should effect the develop command is where it checks for an available port. If it has proceeded to compiling the project, than it is already beyond these changes. The changes that were made in this PR were, ultimately, rather minimal.

However, I will run some tests just to make sure. I will let you know what I come with.

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

@ehowey I can't seem to replicate the problem you're having. I would like to rule out this PR, though, if you don't mind running a test for me. We just need to change a few lines in the source.

In node_modules/gatsby/dist/commands/develop.js, find these lines and comment them out:

...
// Around line 86, comment this line
const detectPortInUseAndPrompt = require(`../utils/detect-port-in-use-and-prompt`);
...
// Around line 355, comment these out
try {
  program.port = await detectPortInUseAndPrompt(port);
} catch (e) {
  if (e.message === `USER_REJECTED`) {
    process.exit(0);
  }
  throw e;
}
// After above is commented out, add this line:
program.port = 8000;

In order to be thorough, we could also change another module. Make it look like below: node_modules/gatsby/dist/utils/detect-port-in-use-and-prompt.js

...
const detectPortInUseAndPrompt = async port => {
/*  let foundPort = port;
  const detectedPort = await detectPort(port).catch(err => report.panic(err));

  if (port !== detectedPort) {
    report.log(`\nSomething is already running at port ${port}`);
    const response = await prompts({
      type: `confirm`,
      name: `newPort`,
      message: `Would you like to run the app at another port instead?`,
      initial: true
    });

    if (response.newPort) {
      foundPort = detectedPort;
    } else {
      throw new Error(`USER_REJECTED`);
    }
  }

  return foundPort;*/
  return 8000;
};

This last one shouldn't be necessary, but I would do it too, just in case. This PR began because of how node was handling stdio buffers, and I'm pretty sure there's some optimizations it does that can cause some strange effects beyond the context of your code, in certain situations.

This completely removes the changes this PR made; there's no reversal... it just bypasses this branch entirely. However, the changes I made moved this branch of code to be on more common footing with the rest of the code base... everything that is in use now, is also used in many other places.

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.