Console class ignores ignoreErrors on Node.js v14.3.0

[exoego]

• Version: Node.js v14.3.0
• Platform: Linux annoMachine 4.18.0-16-generic test: make realpath tests case-insensitive on win32 #17~18.04.1-Ubuntu SMP Tue Feb 12 13:35:51 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: console, stream

What steps will reproduce the bug?

1. Create a repro.js which contents is below:

ws = require('fs').createWriteStream('./foo.log');
ws.destroy();
c = new require('console').Console({
    stdout: ws,
    stderr: ws,
    ignoreErrors: false
});
c.log("should fail");

2. Run ndoe repro.js

How often does it reproduce? Is there a required condition?

Every time it reproduces on the above environment with the above procedure.

What is the expected behavior?

Invoking c.log should throw error, since the underlying streams are already destroyed, like below:

events.js:287
      throw er; // Unhandled 'error' event
      ^

Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed

Note) The expected errors are thrown on Node.js v12.16.3 and v10.20.1.

What do you see instead?

No errors thrown, and script runs succesfully.
It seems that Console class ignores ignorErrors option.

[BridgeAR]

@ronag PTAL

[XadillaX]

It seems because of

ws = require('fs').createWriteStream('./foo.log');
ws.destroy();
ws.write('foo');

do not throw error any more.

[ronag]

This is expected. You can't error an already destroyed stream. #29197

[exoego]

So, errors thrown on Node.js v12 & v10 were bug, that was fixed somewhere on v13/v14 ?

[XadillaX]

But I think it's a breaking change.

[ronag]

But I think it's a breaking change.

Yes, it was labeled as semver-major.

[ronag]

So, errors thrown on Node.js v12 & v10 were bug, that was fixed somewhere on v13/v14 ?

Not sure exactly what you mean. A lot of things fall under that description. But this specific case wasn't a bug per se, but it was ambiguous/undefined behavior that we changed to make it predictable. There were also other problems this caused that the change resolved.

It was a very long discussion as you can notice from the PR and not at decision taken lightly.

[exoego]

Thanks for quick response 👍

I saw there was long disucssion in #29197, and the change seems reasonable to me.
My understanding is that the error I saw on Node.js v12 were due to ambiguous/undefined hehaviors.

I am closing this issue as "as-designed" or similar.