tty issues when switching to raw mode on Windows

[orangemocha]

While investigating nodejs/node#5384 and a host of related issues, we have discovered a few issues and incompatibilites with the current Windows implementation of tty in libuv.

1. Switching to raw mode doesn’t work reliably. This is by far the most severe issue, and the cause of arrow keys not working on Windows per recent user reports. Libuv is relying on CloseHandle to interrupt the call to ReadConsole in another thread. This might have worked on older versions of Windows, but it is unreliable on Windows 7, and doesn’t work at all in Windows 8.1 and higher. On Windows 8.1 and higher, ReadConsole doesn’t return until ENTER is pressed.
2. Even when ReadConsole is interrupted successfully, any keypresses in its buffer are lost forever, whereas on Linux switching to raw mode causes any buffered keypresses to be emitted as raw keypresses.

After much investigation, we have identified two possible fixes:

1. Make ReadConsole return. We have tested CloseHandle, SetConsoleMode, CancelIoEx and none of those do the job reliably. The only reliable way we have found is to send VK_ENTER to the console using WriteConsoleInput, similarly to how libuv already does to cancel the console wait in raw mode.
2. Eliminating the use of ReadConsole and implementing line input in terms of ReadConsoleInput. This essentially amounts to re-implementing a good portion of ReadConsole’s functionality.

Fix a) is much easier, and also the only fix of the two that would be admissible in Node LTS releases, so we are pursuing it first. It doesn’t address the second issue (pending line input getting lost when transitioning to raw mode), but again that seems less critical.

Regarding fix b), I wanted to first get your feedback on whether it would be desirable before deciding if we should attempt it. We would have the ability to emulate the Unix tty behavior more closely, and re-play any pending line input after switching to raw mode (fix issue 2). We would have to keep our own buffer and process arrow keys, backspace, etc.. Line history is implemented by ReadConsole, and doesn’t seem to have a counterpart in the Unix tty implementation. There are a few Node modules implementing history though, including the built-in readline module, so it might not be a bad thing to level this platform difference. The Unix tty also doesn’t handle arrow keys - they are returned as escape sequences. Again, we could consider emulating the same, which would probably also make it easier to implement. Would such behavior be desirable? It would have to be considered a breaking change on Windows.

cc @saghul @piscisaureus

[saghul]

Sounds good to me! As for b), it looks quite complex, I'd got here if we have no other choice, I trust your judgement there. 👍

[piscisaureus]

I know the CloseHandle() trick doesn't work since windows 8 any more (I'm not aware of any reliability issues with Windows 7 though) but as @orangemocha said, there aren't really any good alternative ways to cancel a line buffered read.

Most of the time node got away with it by making sure to never start reading in line buffered mode in the first place; when we start up the repl just switch to raw mode before starting to read on stdin.

But somehow every year or some somehow comes along and makes a change that reverses the order. I find this really an issue with open source (and node in particular) - institutional knowledge is lost so fast. All the subtle mistakes we made and fixed in the repl, url/path path parsing, module loading etc., are made again about every other release, and they will be made again, probably somewhere in the next 12 months, and things will break, and people will complain, and patches will be released, and the cycle will start all over.

Anyway, that was a grumpy tangent.

My advice would be to get rid of line buffered mode entirely. Who needs it anyway?
If not then just land any hack you have in mind.
The only thing I would strongly recommend against is building readline into libuv. It's really not worth it.

[saghul]

Thanks for your insights Bert!

[orangemocha]

Thanks for your input, @piscisaureus !

node got away with it by making sure to never start reading in line buffered mode in the first place

There might have always been a race condition there. Even if you just do process.stdin.setRawMode(true), the mere initialization inside the process.stdin getter runs this code https://github.com/nodejs/node/blob/c490b8ba54df6c730f7bfd02aaafe7c34f571f9e/src/node.js#L665-L716.

The first line in that range triggers a line buffered read, with the following code path:

    node.exe!uv_tty_queue_read_line(uv_loop_s * loop, uv_tty_s * handle) Line 488   C
    node.exe!uv_tty_queue_read(uv_loop_s * loop, uv_tty_s * handle) Line 507    C
    node.exe!uv_tty_read_start(uv_tty_s * handle, void (uv_handle_s *, unsigned int, uv_buf_t *) * alloc_cb, void (uv_stream_s *, int, const uv_buf_t *) * read_cb) Line 937    C
    node.exe!uv_read_start(uv_stream_s * handle, void (uv_handle_s *, unsigned int, uv_buf_t *) * alloc_cb, void (uv_stream_s *, int, const uv_buf_t *) * read_cb) Line 89  C
    node.exe!node::StreamWrap::ReadStart() Line 129 C++
    node.exe!node::StreamBase::ReadStart(const v8::FunctionCallbackInfo<v8::Value> & args) Line 41  C++
    node.exe!node::StreamBase::JSMethod<node::StreamWrap,&node::StreamBase::ReadStart>(const v8::FunctionCallbackInfo<v8::Value> & args) Line 117   C++
     [...]
    at ReadStream.Socket._read (net.js:398:10)
    at ReadStream.Readable.read (_stream_readable.js:336:10)
    at ReadStream.Socket.read (net.js:289:43)
    at ReadStream.Socket (net.js:171:12)
    at new ReadStream (tty.js:27:14)
    at process.stdin (node.js:665:19)

..and the last line cancels the read, in:

    node.exe!uv_tty_read_stop(uv_tty_s * handle) Line 989   C
    node.exe!uv_read_stop(uv_stream_s * handle) Line 107    C
    node.exe!node::StreamWrap::ReadStop() Line 134  C++
    node.exe!node::StreamBase::ReadStop(const v8::FunctionCallbackInfo<v8::Value> & args) Line 46   C++
    node.exe!node::StreamBase::JSMethod<node::StreamWrap,&node::StreamBase::ReadStop>(const v8::FunctionCallbackInfo<v8::Value> & args) Line 117    C++
     [...]
    at process.stdin (node.js:716)

The bug in Node started appearing much more frequently after a c-ares update. My best theory is that the c-ares update caused a change in the timing in Node's startup, in which turn causes the problematic side of the race condition to win the race more often.

[orangemocha]

My advice would be to get rid of line buffered mode entirely. Who needs it anyway?

@piscisaureus I am not sure... is that realistic? Are you suggesting that most users don't read from stdin directly, but rather use the readline module? In any case, it would be a significant breaking change and it would have to go through some deprecation period.

Re-implementing buffered line read in libuv in terms of raw mode would have to be motivated by the opportunity to emulate the unix behavior more closely. I gather from your feedback that is not a very useful goal.

In any case, we'll need to land the hack to make ReadConsole return. A pull request is coming soon.

[piscisaureus]

I am not sure... is that realistic? Are you suggesting that most users don't read from stdin directly, but rather use the readline module?

Well, people read from stdin. But do people really read from the console in line buffered mode?
Note that your typical echo foo | node bla.js is unaffected, since stdin is a pipe in this case and not a console.

Re-implementing buffered line read in libuv in terms of raw mode would have to be motivated by the opportunity to emulate the unix behavior more closely. I gather from your feedback that is not a very useful goal.

It is my sense that it's not a useful goal. But I don't know everything, so if there is evidence to the contrary I will change my opinion.
It'd also be a little weird though as node would end up with two implementations of readline, one in node's readline module and one in libuv.

In any case, we'll need to land the hack to make ReadConsole return. A pull request is coming soon.

👍

[DrPizza]

The Windows 10 update coming some time later this year makes changes of some kind to the console subsystem (the background being the new Linux subsystem which at the very least needs Windows to be as good as a vt100, sigh). I don't know if this will manifest in any API-level changes (though the ability to use ANSI escape codes might be useful for output, at least) but it may make this

Re-implementing buffered line read in libuv in terms of raw mode would have to be motivated by the opportunity to emulate the unix behavior more closely. I gather from your feedback that is not a very useful goal.

more feasible. Or it may not.