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.

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

Ensure async iteration works with destroyed streams #23785

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Oct 20, 2018

This PR is twofold:

  1. 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
  2. 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

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 20, 2018
@mafintosh
Copy link
Member

@mcollina does this supersede my zlib destroy leak PR?

@mcollina
Copy link
Member Author

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

Copy link
Member

@devsnek devsnek left a 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.

if (this[kStream].destroyed) {
// this is needed because if .destro(err) is called, the error
// will be emitted via nextTick
return new Promise((resolve, rejects) => {
Copy link
Member

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?

Copy link
Member Author

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.

@mcollina
Copy link
Member Author

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

@devsnek devsnek added stream Issues and PRs related to the stream subsystem. experimental Issues and PRs related to experimental features. labels Oct 20, 2018
@mafintosh
Copy link
Member

@mcollina this one #23734

@mcollina
Copy link
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.

lib/zlib.js Outdated

Zlib.prototype._destroy = function(err, cb) {
const handle = this._handle;
if (handle && handle.buffer !== null) {
Copy link
Member

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?

lib/zlib.js Outdated
_close(this, callback);
this.destroy();
Zlib.prototype.close = function(callback) {
this.destroy(null, callback);
Copy link
Member

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?

@mcollina
Copy link
Member Author

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

PTAL, this should be ready for review.

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

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

@@ -158,6 +174,9 @@ const createReadableStreamAsyncIterator = (stream) => {
stream.on('readable', onReadable.bind(null, iterator));
stream.on('end', onEnd.bind(null, iterator));
stream.on('error', onError.bind(null, iterator));
// needed because some streams will not emit 'end', e.g. Zlib.
// https://github.com/nodejs/node/issues/23730
stream.on('close', onEnd.bind(null, iterator));
Copy link
Member

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 👍

const passthrough = new PassThrough();
const err = new Error('kaboom');
pipeline(readable, passthrough, common.mustCall((e) => {
assert.strictEqual(err, e);
Copy link
Member

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);

try {
await readable[Symbol.asyncIterator]().next();
} catch (e) {
assert.strictEqual(err, e);
Copy link
Member

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);

@addaleax addaleax removed the zlib Issues and PRs related to the zlib subsystem. label Oct 24, 2018
@addaleax
Copy link
Member

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

@mcollina
Copy link
Member Author

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

PTAL, I've switched to use finished instead.

@lpinca
Copy link
Member

lpinca commented Oct 24, 2018

Still LGTM.

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2018
@mcollina
Copy link
Member Author

Landed in 3ec8cec

@mcollina mcollina closed this Oct 24, 2018
@mcollina mcollina deleted the fix-23730 branch October 24, 2018 13:25
mcollina added a commit that referenced this pull request Oct 24, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@mafintosh
Copy link
Member

(Also still LGTM for the record)

@MylesBorins
Copy link
Contributor

Landed in 10.x with 398418d and b1e1fe4

Please lmk if it should be backed out

@mcollina
Copy link
Member Author

mcollina commented Nov 26, 2018 via email

MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Fixes #23730.

PR-URL: #23785
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. experimental Issues and PRs related to experimental features. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream destruction breaks async-iteration and ends nodejs event loop
10 participants