Skip to content

Commit

Permalink
tty: make _read throw ERR_TTY_WRITABLE_NOT_READABLE
Browse files Browse the repository at this point in the history
This change avoid an 'read ENOTCONN' error introduced by libuv 1.20.0
when trying to read from a TTY WriteStream. Instead, we are throwing
a more actionable ERR_TTY_WRITABLE_NOT_READABLE.

Fixes: nodejs#21203

PR-URL: nodejs#21654
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
mcollina committed Aug 22, 2018
1 parent 6acb550 commit 91eec00
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 1 deletion.
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,12 @@ A `Transform` stream finished with data still in the write buffer.

The initialization of a TTY failed due to a system error.

<a id="ERR_TTY_WRITABLE_NOT_READABLE"></a>
### ERR_TTY_WRITABLE_NOT_READABLE

This `Error` is thrown when a read is attempted on a TTY `WriteStream`,
such as `process.stdout.on('data')`.

<a id="ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET"></a>
### ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET

Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,9 @@ E('ERR_TRANSFORM_ALREADY_TRANSFORMING',
E('ERR_TRANSFORM_WITH_LENGTH_0',
'Calling transform done when writableState.length != 0', Error);
E('ERR_TTY_INIT_FAILED', 'TTY initialization failed', SystemError);
E('ERR_TTY_WRITABLE_NOT_READABLE',
'The Writable side of a TTY is not Readable',
Error);
E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
'`process.setupUncaughtExceptionCapture()` was called while a capture ' +
'callback was already active',
Expand Down
13 changes: 12 additions & 1 deletion lib/tty.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ const { inherits, _extend } = require('util');
const net = require('net');
const { TTY, isTTY } = process.binding('tty_wrap');
const errors = require('internal/errors');
const { ERR_INVALID_FD, ERR_TTY_INIT_FAILED } = errors.codes;
const {
ERR_INVALID_FD,
ERR_TTY_INIT_FAILED,
ERR_TTY_WRITABLE_NOT_READABLE
} = errors.codes;
const { getColorDepth } = require('internal/tty');

// Lazy loaded for startup performance.
Expand Down Expand Up @@ -122,6 +126,13 @@ WriteStream.prototype._refreshSize = function() {
}
};

// A WriteStream is not readable from, so _read become a no-op.
// this method could still be called because net.Socket()
// is a duplex
WriteStream.prototype._read = function() {
this.destroy(new ERR_TTY_WRITABLE_NOT_READABLE());
};

// Backwards-compat
WriteStream.prototype.cursorTo = function(x, y) {
if (readline === undefined) readline = require('readline');
Expand Down
17 changes: 17 additions & 0 deletions test/pseudo-tty/test-tty-write-stream-resume-crash.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

const common = require('../common');
const { WriteStream } = require('tty');
const fd = common.getTTYfd();

// Calling resume on a tty.WriteStream should be a no-op
// Ref: https://github.com/nodejs/node/issues/21203

const stream = new WriteStream(fd);
stream.resume();

stream.on('error', common.expectsError({
code: 'ERR_TTY_WRITABLE_NOT_READABLE',
type: Error,
message: 'The Writable side of a TTY is not Readable'
}));
Empty file.

0 comments on commit 91eec00

Please sign in to comment.