net: emit "write after end" errors in the next tick

[oyyd]

This commit makes those errors caused by calling net.Socket.write() after sockets ending be emitted in the next tick.

This should be "semver-major".

Partially fixes: #24111

Checklist

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

[nodejs-github-bot]

@oyyd sadly an error occured when I tried to trigger a build :(

[oyyd]

CI: https://ci.nodejs.org/job/node-test-pull-request/18720/
CITGM: https://ci.nodejs.org/job/citgm-smoker/1633/

[sam-github]

Why major? All bug fixes are "changes in behaviour", but I think its more likely people have latent buggy code that assumes errors will be in the next tick, code that mysteriously fails on occaison, than that they have code that assumes that the error event is emitted synchronously in this very, very specific timing related scenario.

[Trott]

@nodejs/tsc (on the semver-ness)
@nodejs/citgm (on the CITGM results)

[mcollina]

LGTM

[mcollina]

CITGM seems ok.

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/18800/ (resumed because windows failure)

[Trott]

CITGM and CI are good, this has 2 TSC approvals, and it's been open for more than 48 hours. This can land. @sam-github makes the argument that this shouldn't be semver-major but is instead a bugfix. Thoughts?

[oyyd]

(Sorry for the late reply.)

We also synchronously emit "write after end" errors in streams (which I originally want to handle in this PR):

node/lib/_stream_writable.js

Lines 247 to 252 in d88e26d

	function writeAfterEnd(stream, cb) {
	var er = new ERR_STREAM_WRITE_AFTER_END();
	// TODO: defer error events consistently everywhere, not just the cb
	errorOrDestroy(stream, er);
	process.nextTick(cb, er);
	}

And we have a test for this:

https://github.com/nodejs/node/blob/master/test/parallel/test-file-write-stream.js#L68

than that they have code that assumes that the error event is emitted synchronously in this very, very specific timing related scenario.

I agree with @sam-github, especially in net.Socket but I still think making it semver-major would be safer.

[mcollina]

@oyyd note that via the new autoDestroy: true  feature, the errors will be emitted in the next tick.

I'm ok in landing this in 11.x, and bake it for LTS as long as we need - it might even not be backported at all. However, this is semver-major.

[oyyd]

note that via the new autoDestroy: true feature, the errors will be emitted in the next tick.

👍 Don't know that before.

[Trott]

One way to navigate this is to say it's semver-major but that TSC is allowed to approve landing semver-majors in release lines. (See https://github.com/nodejs/node/blob/e9545e6f203496f886099fb3e93c4d8523cc2378/COLLABORATOR_GUIDE.md#when-breaking-changes-actually-break-things.)

[Trott]

This can land in master at any time. So...

Landed in 9389b46