net: emit "write after end" errors in the next tick by oyyd · Pull Request #24457 · nodejs/node (original) (raw)

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.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation13 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

@oyyd

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

@nodejs-github-bot

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

@oyyd oyyd added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Nov 18, 2018

@oyyd

@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)

addaleax

mcollina

Choose a reason for hiding this comment

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

LGTM

@mcollina

@mcollina

@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):

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

@Trott

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

Landed in 9389b46

Trott pushed a commit to Trott/io.js that referenced this pull request

Nov 25, 2018

@oyyd @Trott

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

PR-URL: nodejs#24457 Fixes: nodejs#24111 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Matteo Collina matteo.collina@gmail.com

refack pushed a commit to refack/node that referenced this pull request

Jan 14, 2019

@oyyd @refack

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

PR-URL: nodejs#24457 Fixes: nodejs#24111 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Matteo Collina matteo.collina@gmail.com

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

This was referenced

Apr 23, 2019

Labels

net

Issues and PRs related to the net subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.