src: Revert nix stdin _readableState.reading

[silverwind]

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]

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]

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]

I tried in Windows 8.1 64bit

[silverwind]

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]

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

[thefourtheye]

Running the fixture alone from powershell does show the issue,

Can't we make that a regression case?

[silverwind]

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

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

[trevnorris]

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]

@trevnorris thanks, will do after some more research.

[thefourtheye]

LGTM as well.

[rvagg]

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]

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]

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

[silverwind]

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

[ChALkeR]

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