Skip to content

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

Closed
richardlau wants to merge 1 commit intonodejs:masterfrom
richardlau:unbreak-build
Closed

Revert "stream: invoke callback before emitting error always"#29741
richardlau wants to merge 1 commit intonodejs:masterfrom
richardlau:unbreak-build

Conversation

@richardlau
Copy link
Member

@richardlau richardlau commented Sep 28, 2019

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

nodejs-github-bot commented Sep 28, 2019

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 72 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

mcollina commented Sep 28, 2019

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
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 72 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