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

Keypress events may not be triggered correctly when keypress event handler contains async invocations (only on Windows) #49588

Open
nicky1038 opened this issue Sep 10, 2023 · 4 comments
Labels
readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform.

Comments

@nicky1038
Copy link

nicky1038 commented Sep 10, 2023

Version

20.6.1

Platform

Microsoft Windows NT 10.0.19045.0 x64 (Windows 10 Pro 22H2)

Subsystem

No response

What steps will reproduce the bug?

The example code is heavily inspired by Prompts library as I've struggled the issue while using this lib. I've also created an issue there, but it seems that the problem is likely in Node.

The example code is roughly re-written Prompts library code that is only responsible for receiving text input, with all irrelevant details omitted. There, the input function waits you to enter some text, records and echoes pressed keys, and when Enter key is pressed, the function finishes its execution and returns what user entered.

Please notice that we pass asyncAction to the first input invocation and only to it — this is the reason of all troubles (though it is expected to work perfectly).

To reproduce the issue:

  • Make sure you're doing this on Windows :)
  • Run this code
  • Enter some text for the first input, make sure that key presses are echoed back, and press Enter
  • Do the same for the second input
  • Start entering text for the third input. Here is where problems begin, key presses are not echoed back as keypress events are not triggered
const { stdin, stdout } = require('process');
const readline = require('readline');

// makes input stream emit 'keypress' events
function createReadline(is, keypress) {
  const rl = readline.createInterface({
    input: is,
    escapeCodeTimeout: 50
  });

  readline.emitKeypressEvents(is, rl);

  if (is.isTTY) {
    is.setRawMode(true);
  }
		
  is.on('keypress', keypress);
	
  // return a callback that stops emitting 'keypress' events and frees resources
  return () => {
    is.removeListener('keypress', keypress);
		
    if (is.isTTY) {
      is.setRawMode(false);
    }
		
    rl.close();
  };
}

// determine what is needed to do for a key object sent by 'keypress' event
function getActionName(key) {
  function needFinish(key) {
    return (key.ctrl && ['c', 'd'].includes(key.name)) ||
      key.name == 'escape' ||
      ['enter', 'return'].includes(key.name);
  }

  function doNothing(key) {
    return ['backspace', 'delete', 'abort', 'escape', 'tab', 'pagedown', 'pageup',
      'home', 'end', 'up', 'down', 'right', 'left'].includes(key.name) || key.ctrl;
  } 
	
  if (doNothing(key)) {
    return 'noop';
  }
  if (needFinish(key)) {
    return 'finish';
  }
  return 'add-letter';
}

// listen to keypresses until user presses Enter, and then return concatenated letters
function input(is, os, onEndInput = () => undefined) {
  return new Promise((resolve) => {
    os.write('Enter string letter by letter, then press Enter\n');
		
    let result = '';
    const closeReadline = createReadline(is, keypress);
		
    function keypress(_, key) {
      os.write(`Encountered ${key.name}\n`)
			
      const actionName = getActionName(key);
			
      if (actionName == 'finish') {
        endInput();
      } else if (actionName == 'add-letter') {
        result += key.name;
      }
    };
		
    async function endInput() {
      await onEndInput(); // <--- If onEndInput is an async function, then it causes the described problem
      closeReadline();
      resolve(result);
    };
  });		
}

async function asyncAction() {
  await new Promise((resolve) => setTimeout(resolve, 100));
  // Another example of async invocation that causes issues (node-fetch@2 is used): await fetch('https://jsonplaceholder.typicode.com/posts');
  // An example of code that does NOT cause issues: await Promise.resolve(0);
}

async function main() {
  await input(stdin, stdout, asyncAction);
  await input(stdin, stdout);
  await input(stdin, stdout);
}

main();

How often does it reproduce? Is there a required condition?

Always reproduced, but only on Windows.
I tried running the example on WSL, no issue there.

What is the expected behavior? Why is that the expected behavior?

All key presses for all user prompts should be triggered correctly and echoed back by the example code

What do you see instead?

When the program awaits user to enter text for the third prompt, no keypress events are triggered, until user presses Enter. And only then, instead of finishing execution, input function starts recording user input.

Additional information

No response

@UsamaBinKashif

This comment was marked as spam.

@bnoordhuis bnoordhuis added readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform. labels Sep 27, 2023
@bnoordhuis
Copy link
Member

^ strong chatgpt vibe

I'm not able to reproduce and it doesn't look like we've received similar reports so I'm inclined to write this off as something local to your setup. (Terminal software maybe?)

If you can track down the root cause, we can decide if it's something node could reasonably handle. Otherwise please go ahead and close this.

@DenyVeyten
Copy link

DenyVeyten commented Oct 28, 2023

I can reproduce the issue by running the provided code.

Windows, Node v16.14.0, Git Bash/PowerShell/cmd

Wrapping closeReadline(); resolve(result); into setTimeout resolves the issue (actually making all subsequent inputs closing async resolves it):

        async function endInput() {
            await onEndInput(); // <--- If onEndInput is an async function, then it causes the described problem
            setTimeout(() => {
                closeReadline();
                resolve(result);
            }, 0);
        }

Wrapping the same into process.nextTick does not resolve it.

Additionally, it's only reproducible for readStream.setRawMode(true).

The other observation is that the issue is actually reproducible for two input requests:

async function main() {
    await input(stdin, stdout, asyncAction);
    await input(stdin, stdout);
}

Here the terminal is waiting for Enter after the second interface is closed

The issue looks related to #38663, where commenting out readStream.setRawMode(false) resolves the issue, but I guess may have some negative impact for the program.

@bnoordhuis
Copy link
Member

Hints for people wanting to investigate:

  • setRawMode(true) calls SetConsoleMode(ENABLE_WINDOW_INPUT) and from then on uses ReadConsoleInput()

  • if you observe ReadConsole() calls instead, that's a bug; should only be called in cooked mode

  • ReadConsole() is called from a helper thread, ReadConsoleInput() from the event loop thread - maybe there's a race condition somewhere?

All the console handling logic is in deps/uv/src/win/tty.c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

4 participants