proposal-iterator-helpers icon indicating copy to clipboard operation
proposal-iterator-helpers copied to clipboard

Does this pass through `return`/`throw` synchronously?

Open glasser opened this issue 3 years ago • 8 comments

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.

glasser avatar Aug 25 '22 21:08 glasser

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

bakkot avatar Aug 25 '22 21:08 bakkot

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

glasser avatar Aug 25 '22 21:08 glasser

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.

Jamesernator avatar Aug 28 '22 08:08 Jamesernator

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.

syg avatar Oct 12 '22 00:10 syg

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.

domenic avatar Oct 14 '22 03:10 domenic

@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 avatar Oct 14 '22 03:10 bakkot

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.

bakkot avatar Oct 14 '22 03:10 bakkot

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.

domenic avatar Oct 14 '22 03:10 domenic

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 avatar Oct 19 '22 17:10 michaelficarra

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.

glasser avatar Oct 19 '22 20:10 glasser

I think we should maybe revisit this as part of https://github.com/tc39/proposal-iterator-helpers/issues/262.

bakkot avatar Jan 26 '23 17:01 bakkot