Skip to content

Revert "stream: do not emit drain if stream ended"#29272

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:revert-drain
Closed

Revert "stream: do not emit drain if stream ended"#29272
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:revert-drain

Conversation

@mcollina

Copy link
Copy Markdown
Member

This reverts commit 605d7c4 following up from the discussion in #29086 (comment).

Please πŸ‘ for fast-track.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes

  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 22, 2019
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina

Copy link
Copy Markdown
Member Author

cc @isaacs

@isaacs

isaacs commented Aug 22, 2019

Copy link
Copy Markdown
Contributor

Digging into this further based on feedback from @ronag. Might have an update in an hour or two.

@isaacs

isaacs commented Aug 22, 2019

Copy link
Copy Markdown
Contributor

I suggest reverting bdf07f4 instead of (or in addition to) this, to fix the TCP drain event emitter warnings. (Via @ronag, and tested locally.)

@isaacs

isaacs commented Aug 22, 2019

Copy link
Copy Markdown
Contributor

Digging into it further, I don't think that 605d7c4 is as much of a problem as I'd originally suspected. I was mistaken. I'm now -1 on this PR, and withdraw my original suggestion. bdf07f4 is definitely the culprit.

@mcollina mcollina closed this Aug 22, 2019
@mcollina

Copy link
Copy Markdown
Member Author

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants