Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: don't emit drain if ended #29086

Closed
wants to merge 1 commit into from

Conversation

@ronag
Copy link
Contributor

commented Aug 11, 2019

Don't emit 'drain' if the stream has been ended. 'drain' should only be emitted if the stream wants more data.

See, https://nodejs.org/api/stream.html#stream_event_drain.

the 'drain' event will be emitted when it is appropriate to resume writing data to the stream.

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

NOTE TO SELF: After this is merged look into emitting 'drain' after state.length < hwm instead of state.length === 0.

@ronag ronag force-pushed the nxtedition:stream-write-drain branch from 8503eb9 to a7d029b Aug 11, 2019

@ronag ronag referenced this pull request Aug 12, 2019
0 of 7 tasks complete
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Looks fine to me, but it would be good to have someone on @nodejs/streams vet it.

Doesn't seem to have semver implications to me but there may be something subtle I'm missing?

Doesn't seem like it would require benchmarking, but again, maybe I'm wrong?

@Trott Trott requested a review from mcollina Aug 16, 2019

@mcollina
Copy link
Member

left a comment

LGTM with a good CITGM run

@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

I’m good with this being a patch.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

I’m good with this being a patch.

Do you still want a CITGM run? (I'm happy to do it if we want to be extra cautious, but also happy to skip it if it's not necessary.)

@Trott
Trott approved these changes Aug 17, 2019
Copy link
Member

left a comment

Making my previous tentative LGTM more official.

@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

@Trott Trott added the author ready label Aug 17, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Those CITGM results seem OK to me after a quick review. You too @mcollina?

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Extra-super-cautious second CITGM: https://ci.nodejs.org/job/citgm-smoker/1969/

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Landed in 605d7c4

@Trott Trott closed this Aug 20, 2019

Trott added a commit that referenced this pull request Aug 20, 2019
stream: do not emit drain if stream ended
PR-URL: #29086
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@isaacs

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

I was mistaken. bdf07f4 is the problem, not this commit. Original comment hidden in details.

This commit is causing problems. I think it should be reverted.

While the docs for 'drain' mention that it's appropriate to write more data, in practice throughout the ecosystem, 'drain' means "all the written data has been consumed", whether the stream is now ended or not.

As a result, .once('drain') handlers are not removed when they should be, and pipelines can even back up if they depend on this behavior. We're seeing this in npm, where users get a flood of "excess event handler" warnings.

Thanks for your attention.

@isaacs

This comment was marked as outdated.

Copy link
Contributor

commented Aug 22, 2019

I think the right thing to do in this case is update the docs, not the code. In a non-semver-major release, that's probably true for any stream behavior mismatch. The ripple effects are huge there.

@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Thanks, we’ll revert this asap.

@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@isaacs have you got a module that broke/had issue about this? I'd love to add a unit test or something to CITGM so we can avoid regressions.

@mcollina mcollina referenced this pull request Aug 22, 2019
1 of 2 tasks complete
@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Makes sense. Is there anything I can do here to follow up? Update the docs to reflect the actual behaviour?

Though I still think the behaviour of this PR is the correct one. Currently it is theoretically possible to cause an write after end in otherwise valid code. I guess we could work around that by updating the docs and also asking the user to check the .writable property after 'drain' and before write(). Not optimal, but maybe sufficient compromise? Or do we revert the revert in a semver-major?

@isaacs

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@mcollina Run npm install against https://github.com/ljharb/es-abstract and you'll see a flood of warnings.

I don't have an example of it backup up a stream and causing problems that way. It'd be harder to reproduce in the wild, because it'd depend on some very specific behavior and timing, but could probably be synthetically created in a test.

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Run npm install against https://github.com/ljharb/es-abstract and you'll see a flood of warnings.

Isn't that the same issue as #29239? i.e. something else?

@isaacs

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@ronag It could be! The issue I saw is that drain isn't getting emitted when it should be, so it led me to this commit, and reverting seems to make the problem go away on my system. But it's possible that it's both, or neither, or something else :)

I'm not sure the best way to go about getting this done, but personally, I'd really like a more massive overhaul of the streams implementation if we're talking about semver major changes. The drain sematic is really not the worst problem.

We could improve performance and vastly simplify the codebase, with a minimal disruption to userland code, by moving to something like minipass. If streams were synchronous, and the class hierarchy rooted on a passthrough implementation (rather than squishing together Readable and Writable into Duplex), then the entire system would be much less brittle. I'm not sure the best way to go about proposing that these days, but I'd be happy to help however I can.

@ronag

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@isaacs: Could you try not reverting this commit and instead revert bdf07f4? I'm just curious whether that makes a difference for you.

@isaacs

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@ronag Will do. I do still think this behavior change is a mistake in a minor version, so probably both need to be rolled back.

@isaacs

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Confirmed @ronag's hunch. Reverting bdf07f4 (or both bdf07f4 and 605d7c4) makes the warnings go away, and reverting only 605d7c4 does not.

ronag added a commit to nxtedition/node that referenced this pull request Aug 23, 2019
stream: do not emit drain if stream ended
PR-URL: nodejs#29086
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
stream: do not emit drain if stream ended
PR-URL: #29086
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: do not emit drain if stream ended
PR-URL: nodejs#29086
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
stream: do not emit drain if stream ended
PR-URL: nodejs#29086
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.