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

src: Revert nix stdin _readableState.reading #3490

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Contributor

This reverts 8cee8f5 which was causing a regression through a possible race condition on stdin on Windows 8 and 10.

Fixes: #2996
Fixes: #2504

The test cases presented in #2996 seem to fail only sometimes, those in #2504 failed all the time for me.

I took one of the examples from #2504 and tried to make a test for it, but it appears to not trigger the bug when spawning in a child process. Running the fixture alone from powershell does show the issue, so I'm a bit out of ideas here. @anseki could you maybe have a look? An automated test for this bug would be great to have.

cc: @chrisdickinson @nodejs/platform-windows

@silverwind silverwind added the stream Issues and PRs related to the stream subsystem. label Oct 22, 2015
@silverwind
Copy link
Contributor Author

According to #2504 (comment), the condition might only show in a real terminal (cmd or powershell). Maybe it's possible to spawn one of these in our test, @anseki.

@anseki
Copy link

anseki commented Oct 25, 2015

Thank you silverwind!
I tried this in following cases the issue existed, and it works fine in all cases:

  • fs.readSync
  • fs.read
  • readline
  • REPL

I hope that this PR is merged.

@anseki
Copy link

anseki commented Oct 25, 2015

I tried in Windows 8.1 64bit

This reverts 8cee8f5 which was causing a regression through a possible
race condition on Windows 8 and 10.

Fixes: nodejs#2996
Fixes: nodejs#2504
@silverwind
Copy link
Contributor Author

I've been unable to produce a working regression test for this issue, the methods used in test-readline-keys and test-repl seem to not work for this case. I've changed the PR to just revert 8cee8f5 now. PTAL.

@silverwind
Copy link
Contributor Author

Anyone able to review? It's a simple revert that fixes 2 (imho) pretty serious issues on Windows.

@silverwind silverwind changed the title stream: fix stdin race condition in Windows 8/10 src: Revert nix stdin _readableState.reading Oct 29, 2015
@thefourtheye
Copy link
Contributor

Running the fixture alone from powershell does show the issue,

Can't we make that a regression case?

@silverwind
Copy link
Contributor Author

@thefourtheye I don't think a automated regression test is possible, stdin needs to be attached to a real terminal like cmd or powershell for it to show, something that seems impossible to do during a test run.

@silverwind
Copy link
Contributor Author

Ping @nodejs/collaborators. This fixes a major bug on Windows, anyone able to review? It's a clean revert of 8cee8f5.

@trevnorris
Copy link
Contributor

Change LGTM.

@silverwind Would you mind updating the commit message and elaborating on "through a possible race condition on stdin"? I'd like to know what I should be looking for in the case I hit this issue, or attempt to write a test in the future.

@silverwind
Copy link
Contributor Author

@trevnorris thanks, will do after some more research.

@thefourtheye
Copy link
Contributor

LGTM as well.

@rvagg
Copy link
Member

rvagg commented Nov 7, 2015

I made a test build of this to make it easier for Windows 8+ users to test, this PR applied to current master: https://nodejs.org/download/test/v6.0.0-test20151107093b0e865c/

Also tested this build with a user here at NodeFest who has experienced this problem with NodeSchool workshoppers (NodeSchool folks are feeling big pain from this) and it works great.

So LGTM!

@silverwind
Copy link
Contributor Author

push('') takes the same code path and does set _readableState.reading to false, so I'm a bit out of ideas on how exactly this bug triggers, the suspected race condition might depend on the timing of when .reading is set. I think at least 4 people have reported fixed issues now, which is enough confirmation for me. Will likely land tomorrow.

@Fishrock123
Copy link
Contributor

@silverwind sounds about right, since .push('') delays it a bit.

silverwind added a commit that referenced this pull request Nov 8, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@silverwind
Copy link
Contributor Author

Landed in af46112. This should definitely be backported to LTS.

@silverwind silverwind closed this Nov 8, 2015
silverwind added a commit that referenced this pull request Dec 4, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
silverwind added a commit that referenced this pull request Dec 17, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
silverwind added a commit that referenced this pull request Dec 23, 2015
This reverts 8cee8f5 which was causing stdin to behave strangely on
Windows 8 and 10. The suspected explanation for the issue is that there
might be a race condition occuring when stdin._readableState.reading is
set indirectly through `push('')`.

PR-URL: #3490
Fixes: #2996
Fixes: #2504
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@ChALkeR
Copy link
Member

ChALkeR commented Jan 27, 2016

There will be no new 3.x releases, per #3465 (comment), removing land-on-v3.x label.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Looks like readline don't always triggers keypress events on Windows Input from TTY is strange
9 participants