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

stream,win: fix ghost keypress events #5776

Closed
wants to merge 2 commits into from

Conversation

orangemocha
Copy link
Contributor

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.
@orangemocha
Copy link
Contributor Author

@mscdex 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
@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2016

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

@calvinmetcalf
Copy link
Contributor

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) ?

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

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

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

What semver rating should this have?

@orangemocha
Copy link
Contributor Author

@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.

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

@orangemocha .. +1

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

@orangemocha .. is this appropriate for v4?

@orangemocha
Copy link
Contributor Author

@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.

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

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

@orangemocha
Copy link
Contributor Author

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

@piranna
Copy link
Contributor

piranna commented Mar 21, 2016

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... :-/

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@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.

@piranna
Copy link
Contributor

piranna commented Mar 21, 2016

@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.

@orangemocha
Copy link
Contributor Author

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.

@piranna piranna mentioned this pull request Mar 22, 2016
4 tasks
orangemocha added a commit that referenced this pull request Mar 23, 2016
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>
orangemocha added a commit that referenced this pull request Mar 23, 2016
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>
@orangemocha
Copy link
Contributor Author

Landed in ace1009 and 4611389. Thank you!

@piranna
Copy link
Contributor

piranna commented Mar 24, 2016

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.

@mafintosh
Copy link
Member

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

@orangemocha
Copy link
Contributor Author

Unfortunately this fix had unwanted consequences: #5927.

@mcollina
Copy link
Member

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.

@mcollina
Copy link
Member

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

@piranna
Copy link
Contributor

piranna commented Mar 29, 2016

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.

@mcollina
Copy link
Member

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.

@piranna
Copy link
Contributor

piranna commented Mar 29, 2016

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.

@mafintosh
Copy link
Member

I agree with @mcollina that we should revert this

@mcollina
Copy link
Member

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.

@orangemocha
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

keypress events not working in windows cmd.exe & powershell
10 participants