console: check that stderr is writable #5635

Trott · 2016-03-09T23:47:26Z

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

console

Description of change

Console constructor checks that stdout.write() is a function but
does not do an equivalent check for stderr.write(). If stderr is not
specified in the constructor, then stderr is set to be stdout.
However, if stderr is specified, but stderr.write() is not a
function, then an exception is not thrown until console.error() is
called.

This change mirrors the check already there for stdout for stderr.
If stderr fails the check, then a TypeError is thrown.

Quite possibly semver-major on the grounds that code out there could
be dependent on the current behavior.

Took the opportunity to copyedit the console doc a little too.

Trott · 2016-03-10T04:34:00Z

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

rvagg · 2016-03-10T11:08:36Z

lib/console.js

+    throw new TypeError('Console expects writable stream instances');
+  }
+
+


extra newline

Fixed. Thanks.

rvagg · 2016-03-10T11:09:57Z

lgtm (I think)

jasnell · 2016-03-11T02:59:23Z

LGTM

Trott · 2016-03-11T16:48:16Z

I know I marked it semver-major to be conservative, but in that spirit, because it's semver-major, it would be great to get a little more CTC sign-off on it before landing. @nodejs/ctc

Trott · 2016-03-14T21:31:21Z

bump @nodejs/collaborators

trevnorris · 2016-03-14T22:08:45Z

lib/console.js

@@ -12,6 +12,10 @@ function Console(stdout, stderr) {
  if (!stderr) {
    stderr = stdout;
  }
+  if (typeof stderr.write !== 'function') {


To be more explicit about what's going on, mind putting this in an if else with the above? Since if stderr == stdout there's no need to check stderr.write.

trevnorris · 2016-03-14T22:30:44Z

This change mirrors the check already there for stdout for stderr.
If stderr fails the check, then a TypeError is thrown.

Mind an alternative wording in the first sentence? Something like "Now instead stderr mirrors stdout's check in the constructor." Key change is the explicit reference to the check being in "the constructor".

Also, this is a semver-minor change. If this was attempted previously:

> var c = new console.Console(process.stdout, {});
> c.log('hi')
hi
> c.error('bye')
TypeError: this._stderr.write is not a function
    at Console.warn (console.js:44:16)

Or the alternative is that they hack around and close the fd. Which has poor results:

> fs.closeSync(process.stderr.fd);
> console.error('foo');
Error: write after end
    at writeAfterEnd (_stream_writable.js:167:12)

So there's no reasonable assumption that existing code would rely on existing behavior. Nothing to worry about. (unless, of course, a bad stream was passed to stderr but was never written to, but have a hard time justifying that we guard that case. if it was the case I'd call this a bug fix b/c it allowed users to write invalid code)

`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too.

Trott · 2016-03-15T02:57:43Z

@trevnorris I've rebased, fixed things up based on your comments, and force pushed. PTAL

Trott · 2016-03-15T22:51:44Z

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

`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too. PR-URL: nodejs#5635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Trott · 2016-03-16T02:14:05Z

Landed in 6ba5af2

`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too. PR-URL: #5635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Trott added semver-major PRs that contain breaking changes and should be released in the next major version. console Issues and PRs related to the console subsystem. labels Mar 9, 2016

Trott force-pushed the console-stderr-check branch from c4c732a to 98816bc Compare March 9, 2016 23:49

rvagg reviewed Mar 10, 2016
View reviewed changes

Trott force-pushed the console-stderr-check branch from a3281f8 to a63bed4 Compare March 10, 2016 18:42

Trott force-pushed the console-stderr-check branch from a63bed4 to 5eba8a2 Compare March 14, 2016 21:30

trevnorris reviewed Mar 14, 2016
View reviewed changes

Trott force-pushed the console-stderr-check branch from 5eba8a2 to d59b431 Compare March 15, 2016 02:57

Trott removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 15, 2016

Trott added the lts-watch-v4.x label Mar 15, 2016

Trott closed this Mar 16, 2016

evanlucas mentioned this pull request Mar 16, 2016

v5.9.0 proposal #5702

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

Trott deleted the console-stderr-check branch January 13, 2022 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console: check that stderr is writable #5635

console: check that stderr is writable #5635

Trott commented Mar 9, 2016

Trott commented Mar 10, 2016

rvagg Mar 10, 2016

Trott Mar 10, 2016

rvagg commented Mar 10, 2016

jasnell commented Mar 11, 2016

Trott commented Mar 11, 2016

Trott commented Mar 14, 2016

trevnorris Mar 14, 2016

trevnorris commented Mar 14, 2016

Trott commented Mar 15, 2016

Trott commented Mar 15, 2016

Trott commented Mar 16, 2016

		throw new TypeError('Console expects writable stream instances');
		}

console: check that stderr is writable #5635

console: check that stderr is writable #5635

Conversation

Trott commented Mar 9, 2016

Affected core subsystem(s)

Description of change

Trott commented Mar 10, 2016

rvagg Mar 10, 2016

Choose a reason for hiding this comment

Trott Mar 10, 2016

Choose a reason for hiding this comment

rvagg commented Mar 10, 2016

jasnell commented Mar 11, 2016

Trott commented Mar 11, 2016

Trott commented Mar 14, 2016

trevnorris Mar 14, 2016

Choose a reason for hiding this comment

trevnorris commented Mar 14, 2016

Trott commented Mar 15, 2016

Trott commented Mar 15, 2016

Trott commented Mar 16, 2016