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.

Sign up for GitHub

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

Jump to bottom

Revert "stream: invoke callback before emitting error always" #29741

Closed
wants to merge 1 commit into from
Closed

Revert "stream: invoke callback before emitting error always" #29741

wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

This reverts commit 3de5eae.

Refs: #29293 (comment)

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 Sep 28, 2019
@richardlau richardlau mentioned this pull request Sep 28, 2019
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25747/

@Trott
Copy link
Member

Trott commented Sep 28, 2019

node-daily-master to confirm master is broken: https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1691/

@Trott
Copy link
Member

Trott commented Sep 28, 2019

node-daily-master to confirm master is broken: https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-master/1691/

It's failing. I propose we fast track this revert. @nodejs/collaborators: Please 👍 here if you approve fast-tracking this revert to fix the build.

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Sep 28, 2019
@Trott Trott mentioned this pull request Sep 28, 2019
Closed
4 tasks
@ronag ronag mentioned this pull request Sep 28, 2019
Closed
4 tasks
@ronag
Copy link
Member

ronag commented Sep 28, 2019 •
edited
Loading

Alternative which fixes bug instead of fully reverting: #29742

@Trott
Copy link
Member

Trott commented Sep 28, 2019

This has sufficient approvals/fast-track/CI to land, but I'd be good with #29742 instead so I'm holding off a bit to see if that can land instead. (But that shouldn't stop someone else from landing this if they think it's the right thing to do or need to fix CI immediately.)

@mcollina
Copy link
Member

I prefer to revert this and have a follow-up PR that readd that behavior.

ronag
ronag reviewed Sep 28, 2019
View reviewed changes
test/parallel/test-zlib-write-after-close.js
type: Error,
message: 'Cannot call write after a stream was destroyed'
}
);
}));
Copy link
Member

@ronag ronag Sep 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This updated test is what's causing the failure. Only reverting this part would also make CI pass.

ronag
ronag approved these changes Sep 28, 2019
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Would prefer #29742 to avoid the administration. But I'm fine with reverting and re-opening either without the failing test update or after #29742.

@Trott
Copy link
Member

Trott commented Sep 28, 2019

Going to land this since it's been a few hours and it's still the only one that's ready to go. Sorry for the bit of extra work that will mean for you, @ronag.

@Trott
Copy link
Member

Trott commented Sep 28, 2019

Landed in 95792a7

@Trott Trott closed this Sep 28, 2019
Trott pushed a commit that referenced this pull request Sep 28, 2019
@richardlau @Trott
Revert "stream: invoke callback before emitting error always"
95792a7 
This reverts commit 3de5eae.

PR-URL: #29741
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@richardlau richardlau deleted the unbreak-build branch September 28, 2019 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@cjihrig cjihrig cjihrig approved these changes

@ronag ronag ronag approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@richardlau @nodejs-github-bot @Trott @ronag @mcollina @cjihrig @gengjiawen @targos