Skip to content

stream: ensure errorEmitted is always set#28709

Closed
ronag wants to merge 1 commit intonodejs:masterfrom
nxtedition:stream-always-set-error-emitted
Closed

stream: ensure errorEmitted is always set#28709
ronag wants to merge 1 commit intonodejs:masterfrom
nxtedition:stream-always-set-error-emitted

Conversation

@ronag
Copy link
Member

@ronag ronag commented Jul 15, 2019

_writableState.errorEmitted should always be set on error.

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jul 15, 2019
@ronag
Copy link
Member Author

ronag commented Jul 15, 2019

@mcollina ping (sorry for spam?)

@ronag ronag force-pushed the stream-always-set-error-emitted branch from 44e5572 to 51428f8 Compare July 15, 2019 22:05
mscdex
mscdex reviewed Jul 15, 2019
View reviewed changes
@ronag ronag force-pushed the stream-always-set-error-emitted branch from 51428f8 to d7bc957 Compare July 15, 2019 22:27
@ronag ronag mentioned this pull request Jul 15, 2019
Closed
@mcollina
Copy link
Member

mcollina commented Jul 16, 2019

Would you mind adding another unit test on the actual behavior change? What consequences has setting errorEmitted in there?

@ronag
Copy link
Member Author

ronag commented Jul 16, 2019

@mcollina: Not sure what you expect there :/

@mcollina
Copy link
Member

mcollina commented Jul 16, 2019

I don't know why you are proposing this change. What's the end user result for this?

@ronag
Copy link
Member Author

ronag commented Jul 16, 2019

@mcollina: Because sometimes errorEmitted is set and sometimes it is not with no clear reason. Why would you not do this change to ensure errorEmitted is (almost) always what you assume it to be?

e.g. _writable_stream:

SET

https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L431
https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L438

NOT SET

https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L252
https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L268
https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L618

@mcollina
Copy link
Member

mcollina commented Jul 16, 2019

+1.

I prefer tests that try to double-emit errors and we block them rather than tests about internals. That's what I'm asking for.

Why would you not do this change to ensure errorEmitted is (almost) always what you assume it to be?

I'm conservative of changes, as every change we do to streams can break a lot of people.

@ronag
Copy link
Member Author

ronag commented Jul 16, 2019 •
edited
Loading

I prefer tests that try to double-emit errors and we block them rather than tests about internals. That's what I'm asking for.

Thank you. Makes sense.

I'm conservative of changes, as every change we do to streams can break a lot of people.

Haha, I guess that is your job.

I'm very keen on consistency and predicability. Unfortunately at the moment it is almost a requirement to read the Node code to figure out how things actually work before using them. I will try to help out within your zone.

@mscdex
Copy link
Contributor

mscdex commented Jul 16, 2019

I agree, we should be running CITGM for the event and stream-related changes in this and the other PRs.

@ronag ronag force-pushed the stream-always-set-error-emitted branch from d7bc957 to 821cc7a Compare July 16, 2019 17:05
@ronag
Copy link
Member Author

ronag commented Jul 16, 2019

@mcollina tests updated

@ronag ronag force-pushed the stream-always-set-error-emitted branch 3 times, most recently from 432d8b2 to a2b670a Compare July 16, 2019 18:09
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 19, 2019

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

mcollina
mcollina requested changes Aug 2, 2019
View reviewed changes
@ronag ronag force-pushed the stream-always-set-error-emitted branch 6 times, most recently from 1c99240 to f018384 Compare August 2, 2019 07:59
mcollina
mcollina approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

This was referenced Aug 6, 2019
Closed
Closed
@ronag ronag mentioned this pull request Aug 6, 2019
Closed
7 tasks
@mcollina
Copy link
Member

mcollina commented Aug 7, 2019

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

1 similar comment
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 7, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 8, 2019

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

@mcollina
Copy link
Member

mcollina commented Aug 8, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1937/

@mcollina
Copy link
Member

mcollina commented Aug 9, 2019

Can you rebase on top of master? CITGM is failing because of that esm bug.

@ronag ronag force-pushed the stream-always-set-error-emitted branch from f018384 to 51d97bf Compare August 9, 2019 11:41
@ronag
Copy link
Member Author

ronag commented Aug 9, 2019

Rebased.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 9, 2019

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

@mcollina
Copy link
Member

mcollina commented Aug 9, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1941

@ronag
Copy link
Member Author

ronag commented Aug 19, 2019

This was fixed in 4a2bd69

@ronag ronag closed this Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@mscdex mscdex mscdex left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

5 participants

@ronag @mcollina @mscdex @nodejs-github-bot @jasnell