stream: give error message if write() cb called twice

[addaleax]

Otherwise, this condition would result in an error that just reads
cb is not a function, and which additionally could have lost
stack trace context through a process.nextTick() call.

$ ./node benchmark/compare.js --new ./node --old ./node-master --filter writable --runs 200 streams | Rscript benchmark/compare.R
[00:09:36|% 100| 1/1 files | 400/400 runs | 1/1 configs]: Done
                                           confidence improvement accuracy (*)   (**)  (***)
 streams/writable-manywrites.js n=2000000                -0.28 %       ±3.51% ±4.62% ±5.92%

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[addaleax]

@nodejs/streams

[cjihrig]

LGTM, but why not just reuse the existing ERR_MULTIPLE_CALLBACK error?

[lpinca]

👍 to @cjihrig's suggestion.

[addaleax]

Yeah, done. Just didn’t know that that existed. :)

[lpinca]

@addaleax I think you forgot to remove ERR_STREAM_WRITE_CALLBACK_TWICE from lib/internal/errors.js in the latest commit.

[mcollina]

LGTM

[mcollina]

I flagged this as semver-major, mainly because it changes the error message and because it uses the new errors (which is semver-major for readable-stream).

[addaleax]

I flagged this as semver-major, mainly because it changes the error message

I would respectfully disagree. As said in the commit message, the previous error message came from trying to call null as a function. That's a VM-generated message, and we haven't generally made guarantees about those.

But more importantly, there is no reason to ever test this error message. It's always a programming error, not some condition that could be detected at runtime.

and because it uses the new errors (which is semver-major for readable-stream).

Can you expand on that? We already use the new errors in the streams code, and it isn't obvious how readable-stream's semver levels relate to Node core's int hese situations_

[mcollina]

Can you expand on that? We already use the new errors in the streams code, and it isn't obvious how readable-stream's semver levels relate to Node core's int hese situations_

My only concern is that this does not get into 6, 8 or 9. This is why I flagged as semver-major. I'm good with do-not-land tags for all release lines as well. I won't land this into the 2.x.x line of readable-stream, and that is currently tracking Node 8.

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/17167/
Re-running linux: https://ci.nodejs.org/job/node-test-commit-linux/17354/

[BridgeAR]

Landed in d111d7b 🎉