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

tty: use blocking mode on OS X #6895

Merged
merged 2 commits into from
Jun 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ test: all
$(MAKE) build-addons
$(MAKE) cctest
$(PYTHON) tools/test.py --mode=release -J \
addon doctool known_issues message parallel sequential
addon doctool known_issues message pseudo-tty parallel sequential
$(MAKE) lint

test-parallel: all
Expand Down Expand Up @@ -183,7 +183,8 @@ test-all-valgrind: test-build
test-ci: | build-addons
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) addons doctool known_issues message parallel sequential
$(TEST_CI_ARGS) addons doctool known_issues message pseudo-tty parallel \
sequential

test-release: test-build
$(PYTHON) tools/test.py --mode=release
Expand Down
6 changes: 5 additions & 1 deletion doc/api/console.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,15 @@ duplicate the browser's functionality exactly.

## Asynchronous vs Synchronous Consoles

The console functions are asynchronous unless the destination is a file.
The console functions are usually asynchronous unless the destination is a file.
Disks are fast and operating systems normally employ write-back caching;
it should be a very rare occurrence indeed that a write blocks, but it
is possible.

Additionally, console functions are blocking when outputting to TTYs
(terminals) on OS X as a workaround for the OS's very small, 1kb buffer size.
This is to prevent interleaving between `stdout` and `stderr`.

## Class: Console

<!--type=class-->
Expand Down
11 changes: 10 additions & 1 deletion doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,8 @@ if (someConditionNotMet()) {
```

The reason this is problematic is because writes to `process.stdout` in Node.js
are *non-blocking* and may occur over multiple ticks of the Node.js event loop.
are usually *non-blocking* and may occur over multiple ticks of the Node.js
event loop.
Calling `process.exit()`, however, forces the process to exit *before* those
additional writes to `stdout` can be performed.

Expand Down Expand Up @@ -1367,6 +1368,10 @@ event and that writes can block when output is redirected to a file (although
disks are fast and operating systems normally employ write-back caching so it
should be a very rare occurrence indeed.)

Additionally, `process.stderr` and `process.stdout` are blocking when outputting
to TTYs (terminals) on OS X as a workaround for the OS's very small, 1kb
buffer size. This is to prevent interleaving between `stdout` and `stderr`.

## process.stdin

A `Readable Stream` for stdin (on fd `0`).
Expand Down Expand Up @@ -1417,6 +1422,10 @@ event and that writes can block when output is redirected to a file (although
disks are fast and operating systems normally employ write-back caching so it
should be a very rare occurrence indeed.)

Additionally, `process.stderr` and `process.stdout` are blocking when outputting
to TTYs (terminals) on OS X as a workaround for the OS's very small, 1kb
buffer size. This is to prevent interleaving between `stdout` and `stderr`.

To check if Node.js is being run in a TTY context, read the `isTTY` property
on `process.stderr`, `process.stdout`, or `process.stdin`:

Expand Down
7 changes: 7 additions & 0 deletions lib/tty.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ function WriteStream(fd) {
writable: true
});

// Prevents interleaved stdout/stderr output in OS X terminals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is correct. The change is intended to fix loosing output when doing process.exit. Interleaving will depend on OS level buffering. Here is the full backstory in case you want to link to it: #6806 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, IMHO the interleaving is a side effect of the blocking-ness.

Copy link
Contributor Author

@Fishrock123 Fishrock123 May 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is intended to fix loosing output when doing process.exit.

Technically it is intended to fix both.

Interleaving will depend on OS level buffering.

Of course, and OS X is hardcoded to 1kb so it won't change regardless.

To clarify, IMHO the interleaving is a side effect of the blocking-ness.

Sure, but it's still a problem we need to cover.

Typically, it would just be an annoyance. You'd have all the information, just sorta mixed up.

But that's not always the case, since people often use ansi control characters in CLI apps.

Consider a problem I ran into while writing the test: At 1024kb of data, including the newline \n, an entire line of data no longer appears.

Why?

Somewhere down the line \n is being changed to \n\r, which overflows the buffer by one character, leaving a separate \r which then erases the current line. Who is going to expect that?

If not me, very few people indeed.
(That was done using Terminal.app on OS X. Given it is the default terminal, this will be in no way isolated.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Carry on then! :-)

// As noted in the following reference, local TTYs tend to be quite fast and
// this behaviour has become expected due historical functionality on OS X,
// even though it was originally intended to change in v1.0.2 (Libuv 1.2.1).
// Ref: https://github.com/nodejs/node/pull/1771#issuecomment-119351671
if (process.platform === 'darwin') this._handle.setBlocking(true);

var winSize = [];
var err = this._handle.getWindowSize(winSize);
if (!err) {
Expand Down
15 changes: 15 additions & 0 deletions test/pseudo-tty/no_dropped_stdio.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// https://github.com/nodejs/node/issues/6456#issuecomment-219320599
// https://gist.github.com/isaacs/1495b91ec66b21d30b10572d72ad2cdd
'use strict';
require('../common');

// 1000 bytes wrapped at 50 columns
// \n turns into a double-byte character
// (48 + {2}) * 20 = 1000
var out = ('o'.repeat(48) + '\n').repeat(20);
// Add the remaining 24 bytes and terminate with an 'O'.
// This results in 1025 bytes, just enough to overflow the 1kb OS X TTY buffer.
out += 'o'.repeat(24) + 'O';

process.stdout.write(out);
process.exit(0);
21 changes: 21 additions & 0 deletions test/pseudo-tty/no_dropped_stdio.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
ooooooooooooooooooooooooO
17 changes: 17 additions & 0 deletions test/pseudo-tty/no_interleaved_stdio.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// https://github.com/nodejs/node/issues/6456#issuecomment-219320599
// https://gist.github.com/isaacs/1495b91ec66b21d30b10572d72ad2cdd
'use strict';
require('../common');

// 1000 bytes wrapped at 50 columns
// \n turns into a double-byte character
// (48 + {2}) * 20 = 1000
var out = ('o'.repeat(48) + '\n').repeat(20);
// Add the remaining 24 bytes and terminate with an 'O'.
// This results in 1025 bytes, just enough to overflow the 1kb OS X TTY buffer.
out += 'o'.repeat(24) + 'O';

const err = '__This is some stderr__';

process.stdout.write(out);
process.stderr.write(err);
21 changes: 21 additions & 0 deletions test/pseudo-tty/no_interleaved_stdio.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
oooooooooooooooooooooooooooooooooooooooooooooooo
ooooooooooooooooooooooooO__This is some stderr__
161 changes: 161 additions & 0 deletions test/pseudo-tty/testcfg.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
# Copyright 2008 the V8 project authors. All rights reserved.
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
# met:
#
# * Redistributions of source code must retain the above copyright
# notice, this list of conditions and the following disclaimer.
# * Redistributions in binary form must reproduce the above
# copyright notice, this list of conditions and the following
# disclaimer in the documentation and/or other materials provided
# with the distribution.
# * Neither the name of Google Inc. nor the names of its
# contributors may be used to endorse or promote products derived
# from this software without specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

import test
import os
from os.path import join, exists, basename, isdir
import re
import utils

FLAGS_PATTERN = re.compile(r"//\s+Flags:(.*)")

class TTYTestCase(test.TestCase):

def __init__(self, path, file, expected, arch, mode, context, config):
super(TTYTestCase, self).__init__(context, path, arch, mode)
self.file = file
self.expected = expected
self.config = config
self.arch = arch
self.mode = mode

def IgnoreLine(self, str):
"""Ignore empty lines and valgrind output."""
if not str.strip(): return True
else: return str.startswith('==') or str.startswith('**')

def IsFailureOutput(self, output):
f = file(self.expected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, preferred way is to use open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied verbatim from test/message/testcfg.py

# Convert output lines to regexps that we can match
env = { 'basename': basename(self.file) }
patterns = [ ]
for line in f:
if not line.strip():
continue
pattern = re.escape(line.rstrip() % env)
pattern = pattern.replace('\\*', '.*')
pattern = '^%s$' % pattern
patterns.append(pattern)
# Compare actual output with the expected
raw_lines = (output.stdout + output.stderr).split('\n')
outlines = [ s.strip() for s in raw_lines if not self.IgnoreLine(s) ]
if len(outlines) != len(patterns):
print "length differs."
print "expect=%d" % len(patterns)
print "actual=%d" % len(outlines)
print "patterns:"
for i in xrange(len(patterns)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterate patterns and outlines directly.

For example,

for pat in patterns:
  ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also copied verbatim from test/message/testcfg.py

print "pattern = %s" % patterns[i]
print "outlines:"
for i in xrange(len(outlines)):
print "outline = %s" % outlines[i]
return True
for i in xrange(len(patterns)):
if not re.match(patterns[i], outlines[i]):
print "match failed"
print "line=%d" % i
print "expect=%s" % patterns[i]
print "actual=%s" % outlines[i]
return True
return False

def GetLabel(self):
return "%s %s" % (self.mode, self.GetName())

def GetName(self):
return self.path[-1]

def GetCommand(self):
result = [self.config.context.GetVm(self.arch, self.mode)]
source = open(self.file).read()
flags_match = FLAGS_PATTERN.search(source)
if flags_match:
result += flags_match.group(1).strip().split()
result.append(self.file)
return result

def GetSource(self):
return (open(self.file).read()
+ "\n--- expected output ---\n"
+ open(self.expected).read())

def RunCommand(self, command, env):
full_command = self.context.processor(command)
output = test.Execute(full_command,
self.context,
self.context.GetTimeout(self.mode),
env,
True)
self.Cleanup()
return test.TestOutput(self,
full_command,
output,
self.context.store_unexpected_output)


class TTYTestConfiguration(test.TestConfiguration):

def __init__(self, context, root):
super(TTYTestConfiguration, self).__init__(context, root)

def Ls(self, path):
if isdir(path):
return [f[:-3] for f in os.listdir(path) if f.endswith('.js')]
else:
return []

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"
" on Windows.")
return result
for test in all_tests:
if self.Contains(path, test):
file_prefix = join(self.root, reduce(join, test[1:], ""))
file_path = file_prefix + ".js"
output_path = file_prefix + ".out"
if not exists(output_path):
print "Could not find %s" % output_path
continue
result.append(TTYTestCase(test, file_path, output_path,
arch, mode, self.context, self))
return result

def GetBuildRequirements(self):
return ['sample', 'sample=shell']

def GetTestStatus(self, sections, defs):
status_file = join(self.root, 'message.status')
if exists(status_file):
test.ReadConfigurationInto(status_file, sections, defs)


def GetConfiguration(context, root):
return TTYTestConfiguration(context, root)
Loading