tty: use blocking mode on OS X

[Fishrock123]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

tty, process (OS X)

Description of change

Refs: #1771
Refs: #6456
Refs: #6773
Refs: #6816

This is a quicker fix on the most broken platform, OS X, while we investigate what needs to be done on other platforms.

The goal is to get at least this into next week's v6 release.

I will try to pull together a test later today. This restores the original, (now) expected, OS X behavior.

[Fishrock123]

@nodejs/ctc I'm resorting to this in case we are not able to determine what to do across all platforms for next week's release.

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

[jasnell]

I'm ok with this as a short term solution.

[Fishrock123]

If anyone could help me with some python for the tests that would be great! Testing branch at https://github.com/Fishrock123/node/tree/test-tty-issues

cc @nodejs/build maybe? @thefourtheye? also cc @addaleax who pointed me to pty.openpty().

[Fishrock123]

Updated with the WIP testcases. The special test runner config still stalls out for me though. :/

[thefourtheye]

Well, preferred way is to use open.

[Fishrock123]

copied verbatim from test/message/testcfg.py

[Fishrock123]

@thefourtheye Any idea why it doesn't work? :S

[Fishrock123]

Also cc @isaacs to see if he is ok with this solution for now?

[Fishrock123]

ping @nodejs/ctc can I please get reviews here? This is quite important for OS X cli users.

Edit: As a note, if we want to have a test with it I'll need help with the python, although that may not actually be necessary for this PR.

[thefourtheye]

@Fishrock123 I am working on fixing the stalling test... I forgot to update here... Sorry :D

[thefourtheye]

@Fishrock123 Can you try this patch.

diff --git a/tools/test.py b/tools/test.py
index 53909ec..874e239 100755
--- a/tools/test.py+++ b/tools/test.py@@ -576,11 +576,17 @@ def RunProcess(context, timeout, args, **rest):
error_mode = SEM_NOGPFAULTERRORBOX;
prev_error_mode = Win32SetErrorMode(error_mode);
Win32SetErrorMode(error_mode | prev_error_mode);
++ faketty = rest.pop('faketty', False)+ pty_out = rest.pop('pty_out')+
process = subprocess.Popen(
shell = utils.IsWindows(),
args = popen_args,
**rest
)
+ if faketty:+ os.close(rest['stdout'])
if utils.IsWindows() and context.suppress_dialogs and prev_error_mode != SEM_INVALID_VALUE:
Win32SetErrorMode(prev_error_mode)
# Compute the end time - if the process crosses this limit we
@@ -592,6 +598,31 @@ def RunProcess(context, timeout, args, **rest):
# loop and keep track of whether or not it times out.
exit_code = None
sleep_time = INITIAL_SLEEP_TIME
+ if faketty:+ output = ''+ while True:+ if time.time() >= end_time:+ # Kill the process and wait for it to exit.+ KillProcessWithID(process.pid)+ exit_code = process.wait()+ timed_out = True+ break++ # source: http://stackoverflow.com/a/12471855/1903116+ # related: http://stackoverflow.com/q/11165521/1903116+ try:+ data = os.read(pty_out, 9999)+ except OSError as e:+ if e.errno != errno.EIO:+ raise+ break # EIO means EOF on some systems+ else:+ if not data: # EOF+ break+ output += data++ return process, process.poll(), timed_out, output+
while exit_code is None:
if (not end_time is None) and (time.time() >= end_time):
# Kill the process and wait for it to exit.
@@ -604,7 +635,7 @@ def RunProcess(context, timeout, args, **rest):
sleep_time = sleep_time * SLEEP_TIME_FACTOR
if sleep_time > MAX_SLEEP_TIME:
sleep_time = MAX_SLEEP_TIME
- return (process, exit_code, timed_out)+ return (process, exit_code, timed_out, '')
def PrintError(str):
@@ -630,27 +661,30 @@ def Execute(args, context, timeout=None, env={}, faketty=False):
if faketty:
(out_master, fd_out) = pty.openpty()
fd_err = fd_out
+ pty_out = out_master
else:
(fd_out, outname) = tempfile.mkstemp()
(fd_err, errname) = tempfile.mkstemp()
+ pty_out = None
# Extend environment
env_copy = os.environ.copy()
for key, value in env.iteritems():
env_copy[key] = value
- (process, exit_code, timed_out) = RunProcess(+ (process, exit_code, timed_out, output) = RunProcess(
context,
timeout,
args = args,
stdout = fd_out,
stderr = fd_err,
- env = env_copy+ env = env_copy,+ faketty = faketty,+ pty_out = pty_out
)
+
if faketty:
- output = os.read(out_master, 9999)
os.close(out_master)
- os.close(fd_out)
errors = ''
else:
os.close(fd_out)

[Fishrock123]

Looks like github is still having problems. I'll run the CI when the branch updates properly. Thanks for all the help @thefourtheye, it passes for me locally, and fails when the patch is removed. :)

[Fishrock123]

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

[Fishrock123]

Crap, looks like pty can't be imported on windows... https://ci.nodejs.org/job/node-test-binary-windows/2235/

@thefourtheye Any ideas?

[thefourtheye]

@Fishrock123 Python 2.x standard library itself doesn't support pty in Windows. I was working with Antti Haapala on this. So cc @ztane.

Can we just skip these tests in Windows?

[Fishrock123]

Can we just skip these tests in Windows?

I would say yes, but given the functionality needs to be part of the test runner I'm not sure how?

[thefourtheye]

We can either do this

diff --git a/test/pseudo-tty/testcfg.py b/test/pseudo-tty/testcfg.py
index 1fdc35a..2e21902 100644
--- a/test/pseudo-tty/testcfg.py+++ b/test/pseudo-tty/testcfg.py@@ -29,6 +29,8 @@ import test
import os
from os.path import join, dirname, exists, basename, isdir
import re
+import utils+
FLAGS_PATTERN = re.compile(r"//\s+Flags:(.*)")
@@ -134,6 +136,9 @@ class TTYTestConfiguration(test.TestConfiguration):
def ListTests(self, current_path, path, arch, mode):
all_tests = [current_path + [t] for t in self.Ls(self.root)]
result = []
+ # Skip these tests on Windows, as pseudo terminals are not available+ if utils.IsWindows():+ print "Skipping pseudo-tty tests, as pseudo terminals are not available in Windows+ return result
for test in all_tests:
if self.Contains(path, test):
file_prefix = join(self.root, reduce(join, test[1:], ""))

or this

diff --git a/tools/test.py b/tools/test.py
index 28fc39b..71e68fb 100755
--- a/tools/test.py+++ b/tools/test.py@@ -1519,13 +1519,8 @@ def Main():
repositories += [TestRepository(a) for a in options.suite]
root = LiteralTestSuite(repositories)
- if len(args) == 0:- paths = [SplitPath(t) for t in BUILT_IN_TESTS]- else:- paths = [ ]- for arg in args:- path = SplitPath(arg)- paths.append(path)+ paths = [SplitPath(t) for t in args or BUILT_IN_TESTS+ if not(t == 'psuedo-tty' and utils.IsWindows())]
# Check for --valgrind option. If enabled, we overwrite the special
# command flag with a command that uses the run-valgrind.py script.

[Fishrock123]

@thefourtheye It's failing on import though:

Traceback (most recent call last):
File "tools\test.py", line 46, in <module>
import pty
File "C:\Python27\lib\pty.py", line 11, in <module>
import tty
File "C:\Python27\lib\tty.py", line 5, in <module>
from termios import *
ImportError: No module named termios

[thefourtheye]

Ah, that can be fixed by simply moving the import pty to the line above ... = pty.openpty()

[thefourtheye]

dirname is not used.

[Fishrock123]

Updated with suggestions, new CI: https://ci.nodejs.org/job/node-test-pull-request/2749/

[Fishrock123]

Updated, added the tests to test-ci. Another run: https://ci.nodejs.org/job/node-test-pull-request/2751/