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

What happens with rejections in synchronous iterables? #16

Closed
senocular opened this issue Dec 16, 2021 · 19 comments
Closed

What happens with rejections in synchronous iterables? #16

senocular opened this issue Dec 16, 2021 · 19 comments
Labels
documentation Improvements or additions to documentation question Further information is requested

Comments

@senocular
Copy link
Contributor

Are synchronous iterables/array-likes being iterated synchronously or asynchronously as they would in a for await? What is the expectation for a rejection within a sync collection that settles before to other promises prior? In a for await it goes unhandled. Would Array.fromAsync() be able to account for that and reject its returned promise or would it to allow for the unhandled rejection to slip through?

const delay = (ms, reject = false) => new Promise((r, j) => setTimeout(reject ? j : r, ms, ms));

for await (let ms of [delay(1000), delay(3000), delay(2000, true)]) {
  console.log(ms) // 1000, (unhandled error 2000), 3000
}
@js-choi js-choi added the question Further information is requested label Dec 16, 2021
@js-choi
Copy link
Collaborator

js-choi commented Dec 16, 2021

Good question. The answer should be yes, the input value’s rejection should be captured returned in the returned promise. That is, Array.fromAsync([delay(1000), delay(3000), delay(2000, true)]) should return a promise that will reject. Each item in the sync iterable is sequentially awaited, and the first item that is a rejection will halt the sequential iteration. At this point, all prior promises in the sequence must have had resolved beforehand, because they would have already been awaited, and all following promises in the sequence are ignored.

I believe that the Array.fromAsync specification reflects this desired behavior.
When we supply a non-async-iterable-but-sync-iterable input to Array.fromAsync,
then, within the Abstract Closure (step 3) of an immediately invoked async function’s promise (steps 2 and 4),
Array.fromAsync will create an Async-from-Sync iterator object in step 3.h.
Each item of this Async-from-Sync iterator object will be Awaited in step 3.j.iii.
If an item settles to a rejection, then that Await operation will cause ThrowCompletion to occur in the async context of the immediately invoked async function from step 4 (see steps 5–7 of the Await algorithm).
In that case, the async function’s promise will reject, as per step 4 and as per AsyncBlockStart’s step 3.f.

Hopefully this answer is satisfactory.
I will add a paragraph to the explainer later that clarifies this.

@js-choi js-choi added the documentation Improvements or additions to documentation label Dec 16, 2021
@senocular
Copy link
Contributor Author

Array.fromAsync will create an Async-from-Sync iterator object in step 3.h.

This is what makes me think it wouldn't handle the rejection. If its iterating through an asynchronous version of the iterator, doesn't it have to wait for the current next value to settle before moving on to the next next value?

In the example array provided, while awaiting delay(3000), delay(2000, true) is going to reject. At this time the loop wouldn't have gotten to that promise to await (step 3.j.iii) because, unless I'm misunderstanding, it wouldn't have called the async iterator's next until delay(3000) resolves.

@js-choi
Copy link
Collaborator

js-choi commented Dec 16, 2021

I think I see what you mean now. Because delay(2000, true) rejects before Array.fromAsync can Await it, Array.fromAsync is unable to add a rejection handler to delay(2000, true) before it does reject.

However, as you know, Awaiting an already-rejected promise will in turn throw an error in the current async context. So my understanding is that Array.fromAsync should still return a promise that will reject at step 3.j.iii once it does reach delay(2000, true), even if delay(2000, true) already has rejected.

Anyways, I will raise this with the proposal’s reviewers (CC: @ljharb, @nicolo-ribaudo) and see if this is truly a problem and if we need to do anything special about this. I would wish for no unhandled rejection to ever escape this function, but there probably is no way to avoid that with for await sequential-read semantics.

Perhaps it is then a matter of whether those input promises count as being “inside” Array.fromAsync (and handled by that function) rather than “outside” Array.fromAsync; if they don’t count as being “inside”, then no rejection can escape Array.fromAsync, because those input promises were never inside Array.fromAsync in the first place.

I suppose we could adopt Promise.all semantics for sync iterables and synchronously dump sync iterables before immediately awaiting all of them in parallel…but that feels pretty strange to me. My intuition is to match for await and sequentially read values, to match semantics with async iterables.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2021

I'm a bit confused. You made a rejected promise and didn't handle it. I'd expect an unhandled rejection here, just like in for-of.

@mhofman
Copy link
Member

mhofman commented Dec 16, 2021

Right, if you generate the promise when consumed by the iterator, it behaves as expected:

const delay = (ms, reject = false) => new Promise((r, j) => setTimeout(reject ? j : r, ms, ms));

function * getIterator (config) {
    for (const item of config) {
        yield delay.apply(null, item);
    }
}

for await (let ms of getIterator([[1000], [3000], [2000, true]])) {
  console.log(ms) // 1000, 3000, (uncaught error 2000)
}

The Async-from-Sync Iterator objects do not eagerly consume the sync iterator. The problem in the original example was that it eagerly created the promises, which included an unhandled rejection.

@senocular
Copy link
Contributor Author

senocular commented Dec 16, 2021

I'm a bit confused. You made a rejected promise and didn't handle it. I'd expect an unhandled rejection here, just like in for-of.

You can catch this in a normal for...of

for (let promise of [delay(1000), delay(3000), delay(2000, true)]) {
   promise.catch(() => 'handled')
}

Promise.all will also handle it

Promise.all([delay(1000), delay(3000), delay(2000, true)]) // rejects, but not unhandled

for-await...of results in an unhandled because it fails to reach the rejection in time.

So the question is, does fromAsync use Promise.all with sync iterables/array-likes (eager), or does it still for-await...of (lazy)? If an iterable is sync, is it meant to be iterated synchronously, even if it contains async (promise) values? Or is fromAsync going to wait and possibly allow for unhandled rejections? If the latter, I think the "supports sync" of this needs a big asterisk saying something to the effect of "you should probably be using Promise.all() instead"

@mhofman
Copy link
Member

mhofman commented Dec 16, 2021

You can catch this in a normal for...of

Right, that's because you eagerly (aka synchronously) consume all the promises produced.

If an iterable is sync, is it meant to be iterated synchronously, even if it contains async (promise) values?

A sync iterable means it synchronously produces values, it doesn't mean it eagerly creates them. Why should a consumer of the iterator assume it can eagerly consume these values?

@ljharb
Copy link
Member

ljharb commented Dec 16, 2021

I would expect that if you did Array.fromAsync([Promise.resolve(), Promise.reject()]) that you'd get an array of two promises, one resolved and one rejected.

The awaiting done here is on the async iteration result, which contains the value but is not itself the value.

@mhofman
Copy link
Member

mhofman commented Dec 16, 2021

To clarify, fromAsync takes an iterable, it doesn't take an array. That's where the distinction is. There is no way to know from just holding an iterable whether it has eagerly created all its values or not, and the safe thing to do is assume it hasn't.

I also doubt we'd want to brand check the argument to see if the iterable is an array or a generic iterable. Now it's possible a generic utility would be useful to turn an array of promises into an async iterable that handles rejections in the way you expect them. For example it'd attach a .catch to all promises eagerly and recreate a new promise for caught unyielded promised on iterator close.

async function * makeAsyncIteratorFromArray(arr) {
  arr = arr.map((value) => Promise.resolve(value));
  arr.forEach((promise) => promise.catch(() => {}));
  let promise;
  try {
    while(promise = arr.shift()) {
      yield await promise;
    }
  } finally {
    while(promise = arr.shift()) {
      new Promise((r) => r(promise));
    }
  }
}
const delay = (ms, reject = false) => new Promise((r, j) => setTimeout(reject ? j : r, ms, ms));

for await (let ms of makeAsyncIteratorFromArray([delay(1000), delay(3000), delay(2000, true)])) {
  console.log(ms) 
  if (ms === 3000) break;
}
// 1000, 3000, (unhandled rejection 2000)

@ljharb
Copy link
Member

ljharb commented Dec 16, 2021

We absolutely must not do that - Array.from takes an iterable, or failing that, an arraylike - it never checks or cares if something's an actual Array. Similarly, Array.fromAsync not check or care - it takes an async iterable, or failing that, an iterable, or failing that, an arraylike.

@senocular
Copy link
Contributor Author

There is no way to know from just holding an iterable whether it has eagerly created all its values or not, and the safe thing to do is assume it hasn't.

Wouldn't the safest thing to do be not try to assume either way?

We can either eagerly consume sync iterators (and array-likes) or not. If we do, we save ourselves from possible unhandled rejections with eagerly created rejecting promises. If not we allow sync iterators to be consumed asynchronously which allows for lazy creation dependent on the results of prior iterations... though if they're implemented as sync iterators, would they even expect to be dependent on that in the first place?

@ljharb
Copy link
Member

ljharb commented Dec 16, 2021

Unhandled rejections aren't something that needs "saving" from. Promise.reject() is a normal part of the language and not some inherent danger.

If you don't want your rejections unhandled, you need to handle them. I don't see how Array.fromAsync needs to take that burden for you, that's not what it's for.

@senocular
Copy link
Contributor Author

If you don't want your rejections unhandled, you need to handle them. I don't see how Array.fromAsync needs to take that burden for you, that's not what it's for.

I want to handle them but through the promise returned by Array.fromAsync as I would be able to for the burdens of:

// These create promises that will reject with err.
Array.fromAsync(badIterable);
Array.fromAsync(genError());
Array.fromAsync(genRejection());
Array.fromAsync(genErrorAsync());
Array.fromAsync([1], badCallback);
BadConstructor.call(Array.fromAsync, []);
// These create promises that will reject with TypeErrors.
Array.fromAsync(null);
Array.fromAsync([], 1);

@mhofman
Copy link
Member

mhofman commented Dec 16, 2021

It sounds like Array.from(await Promise.all(myPromiseArray)) is what you're looking for. There is no reason an unyielded value should magically be handled on your behalf.

@senocular
Copy link
Contributor Author

It sounds like Array.from(await Promise.all(myPromiseArray)) is what you're looking for. There is no reason an unyielded value should magically be handled on your behalf.

But Promise.all is handling it. There's no magic there. It gets yielded, and Promise.all deals with it.

Array.fromAsync could do that too. But should it? That's all I'm asking. Should it or shouldn't it? The way this conversation is going it seems like people are on the side of it shouldn't, that fromAsync is just a direct implementation of for-await...of without any special considerations for sync iterables.

@mhofman
Copy link
Member

mhofman commented Dec 16, 2021

Array.fromAsync could do that too. But should it? That's all I'm asking. Should it or shouldn't it? The way this conversation is going it seems like people are on the side of it shouldn't, that fromAsync is just a direct implementation of for-await...of without any special considerations for sync iterables.

I suppose it could detect if a sync iterable is provided instead of an async iterable, and eagerly consume it. That would indeed be a different behavior than for await of, which I'd find somewhat surprising. At least it wouldn't be a brand check on the type of sync iterable. There is a point to be made that since user code doesn't need to touch individual yielded values, that eager consumption of a sync iterator is acceptable.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2021

It definitely shouldn't - this proposal is not a Promise combinator, it's just condensing an async iterable to an array.

@mhofman
Copy link
Member

mhofman commented Dec 16, 2021

Yeah, the counter argument to eager consumption is that a sync iterable that only generates activity on demand would now be forced to produce all its values. While there is a way too go from array of promise -> async iterator of value while handling rejections, there is no way to unconsume an eagerly consumed sync generator, which would also tilt me in the camp of not eagerly consuming a sync iterable by default.

@js-choi
Copy link
Collaborator

js-choi commented Dec 23, 2021

As I mentioned in #16 (comment), I agree with @ljharb and @mhofman. I think we should continue to use Async-from-Sync iterators (i.e., for await semantics).

The current semantics are such that, if b() will reject before a() resolves, then Array.fromAsync((function * () { yield a(); yield b(); })()) will still eventually return a promise that will eventually reject with b()’s error. However, b’s error intentionally cannot be caught by fromAsync due to its lazy iteration, which makes simultaneously awaiting promises, in parallel, impossible.

The expectation is that, as usual, the creator of the b() promise will catch b()’s rejection wherever b() is created (e.g., at yield b()).

Array.fromAsync already resembles for await in nearly every way, just as Array.from resembles for, and we want to continue this analogy. Like @ljharb says, Array.fromAsync is not intended to be a promise combinator, which would defeat its main purpose for async iterators—instead, it is an operation on potentially lazy iterators. The lazy iterators can be lazy async iterators or lazy sync iterators, but, either way, developers’ mental model is that they are lazy.

Moreover, eager iteration then parallel awaiting is already possible with Promise.all.

The choice between “eager iteration then parallel awaiting” versus “lazy iteration with sequential awaiting” is a crucial and fundamental concern of control flow, and the developer should explicitly decide which they want. Both are useful at different times; both are possible with for loops; but the former is already possible with simple function calls, and the latter is yet not.

Parallel awaiting Sequential awaiting
Lazy iteration Impossible for await (const v of input) f(v);
Eager iteration for (const v of await Promise.all(input)) f(v); Useless
Parallel awaiting Sequential awaiting
Lazy iteration Impossible await Array.fromAsync(input)
Eager iteration await Promise.all(Array.from(input)) Useless

Hopefully this choice is understandable.

I plan to close this issue when I update the explainer to mention this issue.

js-choi added a commit that referenced this issue Dec 27, 2021
js-choi added a commit that referenced this issue Jan 5, 2022
js-choi added a commit that referenced this issue Jan 6, 2022
Closes #18. See also #16.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
js-choi added a commit that referenced this issue Jan 6, 2022
Closes #18. See also #16.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants