8000 stream: ensure errorEmitted is always set by ronag · Pull Request #28709 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    stream: ensure errorEmitted is always set #28709

    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
    Closed
    ronag wants to merge 1 commit into from
    Closed

    stream: ensure errorEmitted is always set #28709

    ronag wants to merge 1 commit into from

    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
    8000

    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
    0