stream,win: fix ghost keypress events by orangemocha · Pull Request #5776 · nodejs/node · GitHub

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

stream, win

Description of change

First commit:
The tty.ReadStream constructor initializes this as a socket,
which causes a read to be initiated. Even though during stdin
initalization we call readStop shortly after, the read operation
can consume keypress events from the system buffers.

Fixes: #5384

Second commit:
The change in the first commit change surfaces an issue with readable streams, which causes test\parallel\test-stdin-resume-pause.js (and a few others) to fail. The second commit addresses that issue.

R: @nodejs/streams @silverwind @cjihrig

The tty.ReadStream constructor initializes this as a socket, which causes a read to be initiated. Even though during stdin initalization we call readStop shortly after, the read operation can consume keypress events from the system buffers. Fixes: nodejs#5384

Readable.resume() schedules the resume operation onto the next tick, whereas pause() has immediate effect. This means that in a sequence stream.resume(); stream.pause(); .. the 'pause' event will be triggered before the resume operation is performed. For process.stdin, we are relying on the 'pause' event to stop reading on the underlying handle. This fix ensures that reads are started and stopped in the same order as resume() and pause() are called.

CI run: https://ci.nodejs.org/job/node-test-pull-request/1950/

LGTM, but would like sign off from a streams person.

are there any tests that cover pausing and then resuming synchronously ? possibly with data being written between the pause and the resume where the state has changed but the event hasn't been emitted, possibly while streaming to something else (or having data streamed to it) ?

@orangemocha ... is there any way to add a test?
LGTM otherwise.

What semver rating should this have?

@jasnell it would be nice to have tests for this sort of things, but it would require some external tooling to generate keystroke events. There are tools that I know work on Windows, I would have to do more research to find out if there are any that work cross platform. I would prefer to defer the general idea of testing console input to @nodejs/testing and not block this PR on it.

The ghost keypress input is a bug fix. The 'resume' event is undocumented and the change shouldn't have any semver-major side effects.

@orangemocha .. +1

@calvinmetcalf I think that https://github.com/nodejs/node/blob/master/test/parallel/test-stream3-pause-then-read.js might provide coverage along the lines of what you are describing, in addition to these other tests
https://github.com/nodejs/node/blob/master/test/parallel/test-stdin-pause-resume-sync.js
https://github.com/nodejs/node/blob/master/test/parallel/test-stdin-pause-resume.js
https://github.com/nodejs/node/blob/master/test/parallel/test-stdin-resume-pause.js

@orangemocha .. is this appropriate for v4?

@jasnell as described in #5384 (comment) the issue is present in all release lines but it becomes more apparent when node startup is slow, which apparently got exacerbated with a c-ares update. The c-ares updated has not been backported to v4.

So I would probably err on the conservative side and not backport it unless we get some reports by v4 users, and not before some bake time in v5 anyway.

@orangemocha +1 .. sounds good! I'll label with dont-land-on-v4 for now.

Sounds good! Still looking for a LGTM by a streams person :)

I've found that this pull-request also fixes #5574, could it be back-ported to Node.js v4.x? That's needed for NodeOS to use the rewritten nsh shell... :-/

@piranna ... potentially but not yet. I'll add the lts-watch label so this is not forgotten about but as @orangemocha recommended this change will need to sit for a bit in v5 first.

@jasnell, thanks for the consideration :-) It's ok for me to know this could eventually be added (NodeOS is not ready to move to Node.js 5 and npm3 yet).

I've found that I don't get the last fragment of text if I write characters "a bit slow" (less than 5550kpm for a text of 996 characters), I'll try to test this pull-request a little bit more to see if it's a bug in my test before submit it.

Barring any objections, I am planning to land this on Wednesday. @nodejs/streams , let me know if you would like to review it but need more time.

The tty.ReadStream constructor initializes this as a socket, which causes a read to be initiated. Even though during stdin initalization we call readStop shortly after, the read operation can consume keypress events from the system buffers. Fixes: #5384 PR-URL: #5776 Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Readable.resume() schedules the resume operation onto the next tick, whereas pause() has immediate effect. This means that in a sequence stream.resume(); stream.pause(); .. the 'pause' event will be triggered before the resume operation is performed. For process.stdin, we are relying on the 'pause' event to stop reading on the underlying handle. This fix ensures that reads are started and stopped in the same order as resume() and pause() are called. PR-URL: #5776 Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

Landed in ace1009 and 4611389. Thank you!

I've found this pull-request doesn't fix the problem I'm having on Ubuntu 32bits and my test doesn't pass on it, I'm going to do more tests to see if I'm having a Linux-only or a 32bits-only problem.

I was away from my computer most of this week so apologise for the late reply. Did anyone test the performance implications of this? .pause() is called by .pipe every time backpressure happens and creating a new closure on every .pause() call to next tick the event could have a negative impact

Unfortunately this fix had unwanted consequences: #5927.

I'm against this fix, in particular the fact that 'pause' is now emitted asynchronously in the next tick. That will have performance implication (you are allocating a clousure) and possibly some unwanted behavior. This should be reverted and discussed again.

I don't think we should change the stream implementation for fixing a problem on windows. Something else must go on.

(I'm sorry to jump later on this, but I have been away for the last 10 days).

I don't think we should change the stream implementation for fixing a problem on windows.

The fix is for Windows, but it (partially) fix the problem I have in #5574 too, and I've check that error happens on Linux and OSX.

This PR changes the behavior of streams to fix a platform specific problem: https://github.com/orangemocha/io.js/blob/f9768a0384acff612666fb12c728c0cac6983706/lib/_stream_readable.js#L742. This is what I question.
The same holds true for stdin: you don't change the stream implementation for the need of a specific subsystem. I don't know if that stream change break some behavior in the userland modules.
Also, this should be labeled at least semver-minor, and maybe semver-major because it changes an existing behavior. And definitely not in LTS.

The fix on windows (and your issue) is probably something different, that does not require changing streams. If you need to change streams, let's discuss it separately, we need to ensure we do not break the API contract entirely, we do not decrease performance, and we do not break modules on NPM.

The fix on windows (and your issue) is probably something different, that does not require changing streams

Ok, I agree, in my case it seems more a bug fix than a change on the behaviour.

I agree with @mcollina that we should revert this

What's the process for reverting something? Maybe the best person to do this is @orangemocha who has the clearer picture. Please cc both @mafintosh and myself.

Thanks for reverting this change, it obviously needs more work.

orangemocha added 2 commits March 18, 2016 02:20

mscdex added windows Issues and PRs related to the Windows platform. tty Issues and PRs related to the tty subsystem. stream Issues and PRs related to the stream subsystem. labels Mar 18, 2016

jasnell added the dont-land-on-v4.x label Mar 18, 2016

santigimeno mentioned this pull request Mar 20, 2016

Losing keystrokes when using tty.ReadStream() over process.stdin #5574

Closed

Fishrock123 mentioned this pull request Mar 21, 2016

up/down keys on Windows have broken behavior in windows #5821

Closed

jasnell added the lts-watch-v4.x label Mar 21, 2016

piranna mentioned this pull request Mar 22, 2016

test: test for issue #5574 #5841

Closed

4 tasks

orangemocha closed this Mar 23, 2016

addaleax mentioned this pull request Mar 27, 2016

Piping from stdin is broken #5927

Closed

evanlucas mentioned this pull request Mar 29, 2016

Revert https://github.com/nodejs/node/pull/5776 #5947

Merged

4 tasks

evanlucas added the dont-land-on-v5.x label Mar 30, 2016

MylesBorins removed the lts-watch-v4.x label Apr 8, 2016

orangemocha mentioned this pull request Apr 14, 2016

Proposal: Require Streams WG to sign off on streams PRs #6178

Closed

bzoz mentioned this pull request Apr 27, 2017

win, pipe: avoid synchronous pipe deadlocks libuv/libuv#1273

Closed

stream,win: fix ghost keypress events by orangemocha · Pull Request #5776 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream,win: fix ghost keypress events - #5776

stream,win: fix ghost keypress events#5776orangemocha wants to merge 2 commits intonodejs:masternodejs/node:masterfrom orangemocha:orangemocha-keypress4orangemocha/io.js:orangemocha-keypress4Copy head branch name to clipboard

Conversation

orangemocha commented Mar 18, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

orangemocha commented Mar 18, 2016

Uh oh!

cjihrig commented Mar 18, 2016

Uh oh!

calvinmetcalf commented Mar 18, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

orangemocha commented Mar 18, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

orangemocha commented Mar 18, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

orangemocha commented Mar 18, 2016

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

orangemocha commented Mar 18, 2016

Uh oh!

piranna commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

piranna commented Mar 21, 2016

Uh oh!

orangemocha commented Mar 21, 2016

Uh oh!

orangemocha commented Mar 23, 2016

Uh oh!

piranna commented Mar 24, 2016

Uh oh!

mafintosh commented Mar 27, 2016

Uh oh!

orangemocha commented Mar 27, 2016

Uh oh!

mcollina commented Mar 29, 2016

Uh oh!

mcollina commented Mar 29, 2016

Uh oh!

piranna commented Mar 29, 2016

Uh oh!

mcollina commented Mar 29, 2016

Uh oh!

piranna commented Mar 29, 2016

Uh oh!

mafintosh commented Mar 29, 2016

Uh oh!

mcollina commented Mar 29, 2016

Uh oh!

orangemocha commented Mar 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

stream,win: fix ghost keypress events#5776
orangemocha wants to merge 2 commits into
nodejs:masterfrom
orangemocha:orangemocha-keypress4