Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Does this pass through return/throw synchronously? #223

Closed
glasser opened this issue Aug 25, 2022 · 11 comments
Closed

Does this pass through return/throw synchronously? #223

glasser opened this issue Aug 25, 2022 · 11 comments

Comments

@glasser
Copy link

glasser commented Aug 25, 2022

One challenge I have found with using async iteration is that it is challenging to do basic functionality like map in a way that works properly with return/throw. For example, it would look like one could implement map like:

async function* map(it, f) {
  for await (const x of it) {
    yield f(x);
  }
}

but calling .return on the value returned from map doesn't actually call it.return immediately if it's blocked on reading from it, which can have poor results. You can build a map that does pass through it.return immediately but it's much harder than writing an async generator. Lots of discussion about that on this issue.

So my question, having recently discovered this proposal, is: does this proposal help with this concern? If you return an async iterator returned by .map(fn) for example, is that passed through to the original iterator immediately, or only if it's currently yielding? I can't figure out the answer myself from README.md or DETAILs.md, and I don't understand the specification well enough to answer it myself there.

@bakkot
Copy link
Collaborator

bakkot commented Aug 25, 2022

If you return an async iterator returned by .map(fn) for example, is that passed through to the original iterator immediately, or only if it's currently yielding?

As currently specified, it works exactly like async generators: calls to .return are queued exactly like calls to .next, and the queue blocks on any internal awaits.

It's an interesting point that the async helpers could be more eager. I'd have to think more about the implications of that. What would you expect to happen to the promise returned by the call to .next which is currently blocking on an await? Would it resolve with { done: true } and discard the result of the await?

(Re: .throw, iterator helpers don't implement .throw at all, pass-through or otherwise.)

@glasser
Copy link
Author

glasser commented Aug 25, 2022

As a use case, imagine an async iterator it that produces a value every second indefinitely until stopped (with return), at which point all pending next()s resolve immediately with { done: true }.

I would hope that it2 = it.map(x => x) would have the exact same behavior: calling it2.return() should immediately call it.return(). But I think it will not?

One issue here. Let's say that when you call it.return() on the initial iterator, it queues some sort of { done: false, value: "finished" } before the { done: true }. I feel like I might want that to be reflected inside it2 as well. But I can also imagine that it2.return() would have to mean "be done right now", not "tell wrapped iterator to be done and be done once it is done". This is a complex API...

@Jamesernator
Copy link

Jamesernator commented Aug 28, 2022

As a use case, imagine an async iterator it that produces a value every second indefinitely until stopped (with return), at which point all pending next()s resolve immediately with { done: true }.

This is as much a hazard as it can be a help. I talked about this on the original issue, basically having this behaviour would make certain operators more footgunny to write.

I think a better model to think about .return() is that .return() DOES NOT MEAN CANCEL, it means NO MORE CALLS TO .next() will be made. i.e. It tells you about future calls to the iterator, but offers no value judgement on past calls (this is precisely why async generators queue).

Like what you want here is cancellation, as you're essentially notifying multiple calls that their results no longer matter. It is rather a large shame the cancellation proposal has floundered and host APIs like AbortSignal don't integrate with anything in the language.

@bakkot bakkot changed the title Does this pass through return/throw properly? Does this pass through return/throw synchronously? Sep 2, 2022
@syg
Copy link

syg commented Oct 12, 2022

I don't have too much of a language design opinion here, but having iterator helpers that can mostly reuse async generator machinery except for return/throw might be annoying for implementation complexity.

If there's no definitive compelling use case agreed upon by committee, and the reasoning to support synchronous return and throw is more along the lines of "sure why not", I'd err on the side of letting iterator helpers be explained by async generators.

For V8 it might not be too bad, but probably annoying. For self-hosting implementations, I wonder if having synchronous return and throw make it much more annoying to implement.

@domenic
Copy link
Member

domenic commented Oct 14, 2022

It seems pretty weird to solve this problem for iterator-helper-derived async iterators, and not other generator-derived async iterators? Like, will we expect people to stop using raw generators, and instead always write a wrapper like

function betterAsyncGenerator(...args) {
  return originalAsyncGenerator(...args).map(x => x);
}

so that they can get this early-abort functionality?

It feels like a better level at which to solve this would be to add some method to all generator-derived async iterators, e.g. makeReturnAndThrowEager(), which people could use on any generator-derived async iterator.

@bakkot
Copy link
Collaborator

bakkot commented Oct 14, 2022

@domenic The proposed change here wouldn't mean that iterator-helper-derived async iterators had this functionality when other iterators did not. originalAsyncGenerator(...args).map(x => x) wouldn't really do anything different either way, because the underlying async generator would still put the next/return calls into a queue even if the iterator helper wrapper was able to pass through those calls before waiting for promises from earlier calls settle.

Rather, the proposed change would be to preserve this functionality when the underlying iterator already supported it. As currently spec'd, even if you have a manually-implemented async iterator which can handle a call to .return while blocked on a call to .next, doing let it = manualIterator.map(x=>x); it.next(); it.return(); will wait for the call to .next to complete before the underlying iterator sees the call to .return at all.

@bakkot
Copy link
Collaborator

bakkot commented Oct 14, 2022

That said, I think we're currently leaning towards not making any changes to the current spec, for the sake of staying consistent with async generators. Even though that means a manually-implemented async iterator which is capable of eagerly handling calls to .return will lose that functionality when wrapped by an iterator helper.

@domenic
Copy link
Member

domenic commented Oct 14, 2022

originalAsyncGenerator(...args).map(x => x) wouldn't really do anything different either way

Got it, that makes sense. And so I guess my "add a method" idea doesn't work.

The proposed change here wouldn't mean that iterator-helper-derived async iterators had this functionality when other iterators did not.

Right. I'm saying it means that iterator-helper-derived async iterators, but other generator-derived async iterators do not.

So I guess it just makes writing your own generators inferior to cobbling one together out of combinators, which is... slightly bad?

I still think addressing this more generally for all generator-derived async iterators would be better, but now that I see the problem more clearly I guess it's not very easy. The best I can come up with is something like yield.interruptable. Then you could say that iterator helpers use yield.interruptable internally, and in general give the advice to use yield.interruptable when writing combinators and yield when writing sources.

@michaelficarra
Copy link
Member

Looks like we're all pretty much in agreement here. Let's only preserve the functionality of the underlying iterator that an async-generator-based helper implementation could preserve. Thanks for prompting this discussion @glasser.

@michaelficarra michaelficarra closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2022
@glasser
Copy link
Author

glasser commented Oct 19, 2022

It's a bummer that these helpers won't solve this use case, but I guess the bummer is more about the overall experience of using async iteration than about the helpers specifically, so that makes sense.

@bakkot
Copy link
Collaborator

bakkot commented Jan 26, 2023

I think we should maybe revisit this as part of #262.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants