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

stream: fix handling generators in Readable.from #32844

Closed

Conversation

vadzim
Copy link
Contributor

@vadzim vadzim commented Apr 14, 2020

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

closes #32842

@vadzim vadzim changed the title Fix handling generators in readable from Fix handling generators in stream.Readable.from Apr 14, 2020
@ronag ronag requested review from mcollina and ronag April 14, 2020 13:51
lib/internal/streams/from.js Outdated Show resolved Hide resolved
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.

Good spot. I'm not sure needToClose is required, just always call return() in _destroy(). Also, please add a test.

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.

Would you mind to add a unit test for this? Thanks

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

@ronag @mcollina sure, I'll add a test as soon as it builds on my machine.

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

I'm not sure needToClose is required, just always call return() in _destroy()

for-of loop does this check. Why we shouldn't?

@ronag
Copy link
Member

ronag commented Apr 14, 2020

Here is a "desugared" pseudo version of for-of I got from Gus Caplan:

const iterator = B[Symbol.asyncIterator]();
while (true) {
  const next = await iterator.next();
  if (next.done) {
    break;
  }
  const A = next.value;

  // pseudocode follows, because dealing with `break` and `throw` is difficult to desugar.
  const result = BODY;
  if (result is a break or result is a throw) {
    if (iterator.return !== undefined) {
      let innerResult = iterator.return();
      if (innerResult is not a throw) {
        // if iterator.return() threw an exception, we don't want to await it.
        innerResult = await innerResult;
      }
      if (result is a throw) {
        // if BODY threw an exception, we want to throw that one instead of the possible
        // exception from iterator.return()
        throw result;
      }
      if (innerResult is a throw) {
        // if iterator.return() threw, either directly or because it returned a rejected promise,
        // now we re-throw that exception.
        throw innerResult;
      }
      break;
    }
  }
}

I think that's probably what we should aim for.

As you've noticed the throw + return() part is tricky.

@ronag
Copy link
Member

ronag commented Apr 14, 2020

for-of loop does this check.

Where do you see this? Maybe?

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

I'm sorry, it's quite difficult for me to understand that pseudocode.
The next is what I got from the node cli:

// next returns ordinary value, done = false:
for(const x of {
  [Symbol.iterator](){ return this },
  next(){ console.log('next'); return { value: 42, done: false } },
  return(){ console.log('return'); return { done: true } }
}) {
  console.log(x)
  break
}
// output:
//    next
//    42
//    return
//    undefined

// next returns done = true:
for(const x of {
  [Symbol.iterator](){ return this },
  next(){ console.log('next'); return { value: 42, done: true } },
  return(){ console.log('return'); return { done: true } }
}) {
  console.log(x)
  break
}
// output:
//    next
//    undefined

// next throws
for(const x of {
  [Symbol.iterator](){ return this },
  next(){ console.log('next'); throw 42 },
  return(){ console.log('return'); return { done: true } }
}) {
  console.log(x)
  break
}
// output:
//    next
//    Uncaught 42

It seems that return isn't called if next throws.

lib/internal/streams/from.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member

ronag commented Apr 14, 2020

The next is what I got from the node cli:

What do you want to illustrate with this?

It seems that return isn't called if next throws.

Yes, that's what we are fixing in this PR?

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

What do you want to illustrate with this?

I mean

It seems that return isn't called if next throws.

@ronag
Copy link
Member

ronag commented Apr 14, 2020

Ah, I think I understand what you mean.

@ronag
Copy link
Member

ronag commented Apr 14, 2020

What happens if you do the same thing with async iterators?

@ronag
Copy link
Member

ronag commented Apr 14, 2020

It seems that return isn't called if next throws.

But then the problem with #32842 remains?

I think looking at the for await..of would be relevant.

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

What happens if you do the same thing with async iterators?

Same thing happens if we use for-await-of with async iterator.
.return isn't called if .next throws or returns {done: true}.

@ronag

This comment has been minimized.

lib/internal/streams/from.js Outdated Show resolved Hide resolved
@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

Giving this more thought. I think the absolute simplest and most correct implementation is to just simply use a for..of.

We still need to deal with buffer in readable stream.
After readable.push returns false we should wait for the next read call.
We just need _destroy implementation.

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.

LGTM once tests are added, good job!

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Apr 14, 2020
@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

I'll write the tests, though it can take some time cause I'm gonna cover all these mentioned edge cases.

@ronag
Copy link
Member

ronag commented Apr 14, 2020

Also, please take a look at the commit guidelines.

You should use stream: as subsystem, and Fixes: before the url for the issue it fixed.

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

Sure, I'll squash my draft commits into one when the job is done.

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

I've added tests and it's allowed me to catch one more case.
But I'm afraid those tests are too complex to support and now I like your proposal to use for-of cycle. I'm gonna to prepare another PR to compare.

@vadzim
Copy link
Contributor Author

vadzim commented Apr 14, 2020

Actually the code does not look simpler) and the tests should remain in any case.
But you can choose either #32855

@vadzim vadzim force-pushed the FixHandlingGeneratorsInReadableFrom branch from 426b1d6 to 6bddcf7 Compare April 14, 2020 22:29
@vadzim vadzim requested a review from ronag April 15, 2020 13:46
lib/internal/streams/from.js Outdated Show resolved Hide resolved
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.

LGTM

@nodejs-github-bot
Copy link
Collaborator

@vadzim vadzim requested a review from mcollina April 16, 2020 07:55
@vadzim vadzim changed the title Fix handling generators in stream.Readable.from stream: fix handling generators in Readable.from Apr 17, 2020
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

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 17, 2020
@nodejs-github-bot
Copy link
Collaborator

@vadzim
Copy link
Contributor Author

vadzim commented Apr 17, 2020

@ronag @mcollina
Thank you for the great review. It's really valueble for my experience.

Btw, actually the code didn't contain any iterator.return() call, so it didn't close the iterator when not of its values were consumed. E.g. when the stream is destroyed.

I guess this bugfix could be applied to node@12. Does it make sense?

@mcollina
Copy link
Member

Absolutely, in due time. It will be included in the closest release of 12.x after 2 weeks of being in 13.x.

ronag pushed a commit that referenced this pull request Apr 18, 2020
Call iterator.return() if not all of its values are consumed.

Fixes: #32842

PR-URL: #32844
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@ronag
Copy link
Member

ronag commented Apr 18, 2020

Landed in 8a3fa32

@ronag ronag closed this Apr 18, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
Call iterator.return() if not all of its values are consumed.

Fixes: #32842

PR-URL: #32844
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Call iterator.return() if not all of its values are consumed.

Fixes: #32842

PR-URL: #32844
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request May 7, 2020
Call iterator.return() if not all of its values are consumed.

Fixes: #32842

PR-URL: #32844
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
targos pushed a commit that referenced this pull request May 13, 2020
Call iterator.return() if not all of its values are consumed.

Fixes: #32842

PR-URL: #32844
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@benjamingr
Copy link
Member

Hi, I don't understand why needToClose is here? If someone destroys the stream we need to always close the generator don't we?

@benjamingr
Copy link
Member

That is - if someone creates a stream, calls .read but .next is never called on the generator - needToClose is false and blocks don't run (since await iterator.next()` is async and waits a microtick)

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. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stream.Readable.from does not always close synchronous generators
6 participants