GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Ensure async iteration works with destroyed streams by mcollina · Pull Request #23785 · nodejs/node

Conversation

mcollina commented Oct 20, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This PR is twofold:

~~makes sure that .destroy() works correctly on zlib streams (before it was not cleaning up the _handle)~~ done in zlib: do not leak on destroy #23734
fixes support for async-iterating destroyed streams.

Fixes #23730 .

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mcollina requested review from addaleax, devsnek, jasnell and mafintosh

October 20, 2018 13:09

nodejs-github-bot commented Oct 20, 2018

Copy link

Copy Markdown

Collaborator

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1317/pipeline

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added the zlib Issues and PRs related to the zlib module and its compression dependencies. label

Oct 20, 2018

mcollina mentioned this pull request

Oct 20, 2018

Stream destruction breaks async-iteration and ends nodejs event loop #23730

Closed

jasnell reviewed

Oct 20, 2018

View reviewed changes

Comment thread

test/parallel/test-stream-readable-async-iterators.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

mafintosh commented Oct 20, 2018

Copy link

Copy Markdown

Member

@mcollina does this supersede my zlib destroy leak PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Hakerh400 commented Oct 20, 2018

Copy link

Copy Markdown

Contributor

https://github.com/nodejs/node/blob/0983ed818e5b93b6db794b04b58425d8d4ffc43d/lib/internal/streams/async_iterator.js#L90-L91

Nit: destroy(err)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina commented Oct 20, 2018

Copy link

Copy Markdown

Member Author

@mafintosh which one? Very likely that should be merged first.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

devsnek approved these changes

Oct 20, 2018

View reviewed changes

devsnek left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It seems this relies on a race condition? I won't pretend to know anything about our crazy streams but I want to just verify what's going on here is what is intended.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/streams/async_iterator.js Outdated

devsnek Oct 20, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

any reason for rejects rather than reject?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mcollina Oct 20, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

none, I’ll fix it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mcollina commented Oct 20, 2018

Copy link

Copy Markdown

Member Author

@devsnek this is normalizing a race condition that we cannot get rid of inside our stream machinery :/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

devsnek added stream

Issues and PRs related to Node.js streams.

experimental Issues and PRs related to experimental features. labels

Oct 20, 2018

mafintosh commented Oct 20, 2018

Copy link

Copy Markdown

Member

@mcollina this one #23734

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina commented Oct 20, 2018

Copy link

Copy Markdown

Member Author

Ah I didn’t know we were working on essentially the same issue!

Can you pick up the change that I did inside processCallback? It might even be worth a test on its own. I’ll rebase this on top of yours.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca reviewed

Oct 20, 2018

View reviewed changes

Comment thread

lib/internal/streams/async_iterator.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

addaleax reviewed

Oct 21, 2018

View reviewed changes

Comment thread

lib/zlib.js Outdated

addaleax Oct 21, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you add a comment here describing what this condition means?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/zlib.js Outdated

addaleax Oct 21, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This changes timing for the close callback… is that intentional?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax mentioned this pull request

Oct 21, 2018

zlib: do not leak on destroy #23734

Closed

2 of 4 tasks

mcollina force-pushed the fix-23730 branch from c13cc44 to 0d029f6 Compare

October 23, 2018 22:52

mcollina commented Oct 23, 2018

Copy link

Copy Markdown

Member Author

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

PTAL, this should be ready for review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina force-pushed the fix-23730 branch from 0d029f6 to 80487d0 Compare

October 24, 2018 09:27

mcollina commented Oct 24, 2018

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina commented Oct 24, 2018

Copy link

Copy Markdown

Member Author

@lpinca @addaleax is this ok to land for you?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mafintosh reviewed

Oct 24, 2018

View reviewed changes

Comment thread

lib/internal/streams/async_iterator.js Outdated

mafintosh Oct 24, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

could use stream.finished to simplify in the future, but 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mafintosh approved these changes

Oct 24, 2018

View reviewed changes

lpinca approved these changes

Oct 24, 2018

View reviewed changes

Comment thread

test/parallel/test-stream-readable-async-iterators.js Outdated

lpinca Oct 24, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert.strictEqual(err, e);
	assert.strictEqual(e, err);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/parallel/test-stream-readable-async-iterators.js Outdated

lpinca Oct 24, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert.strictEqual(err, e);
	assert.strictEqual(e, err);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax removed the zlib Issues and PRs related to the zlib module and its compression dependencies. label

Oct 24, 2018

addaleax commented Oct 24, 2018

Copy link

Copy Markdown

Member

@mcollina There’s nothing speaking against landing this as far as I am concerned :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          stream: async iteration should work with destroyed stream

14586b0

Fixes nodejs#23730.

mcollina force-pushed the fix-23730 branch from 80487d0 to 14586b0 Compare

October 24, 2018 11:08

mcollina commented Oct 24, 2018

Copy link

Copy Markdown

Member Author

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

PTAL, I've switched to use finished instead.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Oct 24, 2018

Copy link

Copy Markdown

Member

Still LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mmarchini approved these changes

Oct 24, 2018

View reviewed changes

mmarchini added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Oct 24, 2018

mcollina commented Oct 24, 2018

Copy link

Copy Markdown

Member Author

Landed in 3ec8cec

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina closed this

Oct 24, 2018

mcollina deleted the fix-23730 branch

October 24, 2018 13:25

mcollina added a commit that referenced this pull request

Oct 24, 2018


          stream: async iteration should work with destroyed stream

3ec8cec

Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>

mafintosh commented Oct 24, 2018

Copy link

Copy Markdown

Member

(Also still LGTM for the record)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos pushed a commit that referenced this pull request

Oct 24, 2018


          stream: async iteration should work with destroyed stream

aaddf97

Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>

This was referenced Oct 26, 2018

Stream async iteration breaks when the loop body calls .destroy(null) #23890

Closed

Multiple fixes for async iterators #23901

Closed

targos mentioned this pull request

Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

This was referenced Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248

Closed

MylesBorins added dont-land-on-v8.x labels

Nov 26, 2018

MylesBorins commented Nov 26, 2018

Copy link

Copy Markdown

Contributor

Landed in 10.x with 398418d and b1e1fe4

Please lmk if it should be backed out

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina commented Nov 26, 2018 via email

Copy link

Copy Markdown

Member Author

go for it! Il giorno lun 26 nov 2018 alle 22:41 Myles Borins <[email protected]> ha scritto:

…

Landed in 10.x with 398418d <398418d> and b1e1fe4 <b1e1fe4> Please lmk if it should be backed out — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#23785 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL4-2fOTTylhaBVvajyK70fXegKpuvks5uzGAUgaJpZM4Xx6oH> .

MylesBorins reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins pushed a commit that referenced this pull request

Nov 26, 2018


          stream: async iteration should work with destroyed stream

dbd37ab

Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>

codebytere mentioned this pull request

Nov 27, 2018

v10.13.1 proposal #24675

Closed

rvagg pushed a commit that referenced this pull request

Nov 28, 2018


          stream: async iteration should work with destroyed stream

fca312a

Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Nov 29, 2018


          stream: async iteration should work with destroyed stream

ce79ae6

Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>

codebytere mentioned this pull request

Nov 29, 2018

v10.14.2 proposal #24727

Merged

jaydenseric mentioned this pull request

Jan 10, 2019

Errors in tests appearing for Node.js >= v10.14.2 jaydenseric/graphql-upload#130

Closed

rektide mentioned this pull request

Mar 6, 2019

Advance Readable[Symbol.asyncIterator] out of experimental #26479

Closed

Reviewers

jasnell

jasnell left review comments

addaleax

addaleax left review comments

lpinca

lpinca approved these changes

+3 more reviewers

mafintosh

mafintosh approved these changes

mmarchini

mmarchini approved these changes

devsnek

devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

experimental

Issues and PRs related to experimental features.

stream

Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Stream destruction breaks async-iteration and ends nodejs event loop

Uh oh!

There was an error while loading. Please reload this page.

10 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

mcollina commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 20, 2018

Uh oh!

Uh oh!

mafintosh commented Oct 20, 2018

Uh oh!

Hakerh400 commented Oct 20, 2018

Uh oh!

mcollina commented Oct 20, 2018

Uh oh!

devsnek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 20, 2018

Uh oh!

mafintosh commented Oct 20, 2018

Uh oh!

mcollina commented Oct 20, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 23, 2018

Uh oh!

mcollina commented Oct 24, 2018

Uh oh!

mcollina commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 24, 2018

Uh oh!

mcollina commented Oct 24, 2018

Uh oh!

lpinca commented Oct 24, 2018

Uh oh!

mcollina commented Oct 24, 2018

Uh oh!

mafintosh commented Oct 24, 2018

Uh oh!

MylesBorins commented Nov 26, 2018

Uh oh!

mcollina commented Nov 26, 2018 via email

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mcollina commented Oct 20, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.