Skip to content

console: check that stderr is writable #5635

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

console: check that stderr is writable #5635

wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

@Trott Trott commented Mar 9, 2016

  • 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 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 Trott force-pushed the console-stderr-check branch from c4c732a to 98816bc Compare March 9, 2016 23:49
@Trott
Copy link
Member Author

Trott commented Mar 10, 2016

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

rvagg
rvagg reviewed Mar 10, 2016
View reviewed changes
lib/console.js Outdated
Copy link
Member

@rvagg rvagg Mar 10, 2016

Choose a reason for hiding this comment

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

extra newline

Copy link
Member Author

@Trott Trott Mar 10, 2016

Choose a reason for hiding this comment

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

Fixed. Thanks.

@rvagg
Copy link
Member

rvagg commented Mar 10, 2016

lgtm (I think)

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

jasnell commented Mar 11, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Mar 11, 2016

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 Trott force-pushed the console-stderr-check branch from a63bed4 to 5eba8a2 Compare March 14, 2016 21:30
@Trott
Copy link
Member Author

Trott commented Mar 14, 2016

bump @nodejs/collaborators

trevnorris
trevnorris reviewed Mar 14, 2016
View reviewed changes
lib/console.js Outdated
Copy link
Contributor

@trevnorris trevnorris Mar 14, 2016

Choose a reason for hiding this comment

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

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
Copy link
Contributor

trevnorris commented Mar 14, 2016

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)

@Trott
console: check that stderr is writable
d59b431 
`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 Trott force-pushed the console-stderr-check branch from 5eba8a2 to d59b431 Compare March 15, 2016 02:57
@Trott Trott removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 15, 2016
@Trott
Copy link
Member Author

Trott commented Mar 15, 2016

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

@Trott
Copy link
Member Author

Trott commented Mar 15, 2016

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

Trott added a commit to Trott/io.js that referenced this pull request Mar 16, 2016
@Trott
console: check that stderr is writable
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: nodejs#5635
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@Trott
Copy link
Member Author

Trott commented Mar 16, 2016

Landed in 6ba5af2

@Trott Trott closed this Mar 16, 2016
evanlucas pushed a commit that referenced this pull request Mar 16, 2016
@Trott @evanlucas
console: check that stderr is writable
5e9cac4 
`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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 16, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@Trott
console: check that stderr is writable
9c3c6e7 
`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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@Trott
console: check that stderr is writable
0c6f674 
`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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@Trott Trott deleted the console-stderr-check branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

console Issues and PRs related to the console subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Trott @rvagg @jasnell @trevnorris @MylesBorins