Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Where should value unwrapping be performed? #15

Closed
zenparsing opened this issue Dec 14, 2015 · 74 comments
Closed

Where should value unwrapping be performed? #15

zenparsing opened this issue Dec 14, 2015 · 74 comments

Comments

@zenparsing
Copy link
Member

In #8, we agreed that when you yield a promise:

async function* g() {
    yield new Promise(...);
}

The promise should be unwrapped before being returned to the consumer.

In that thread, we implicitly agreed that within async generators, yield x should evaluate like yield await x: in other words, the value is unwrapped on the producer side. This seems good.

However, we can still manually create an async iterator that returns iteration results where the value component is a promise.

let manualAsyncGen = {
    async next() {
        return { value: timeoutPromise("Hello world", 500), done: false };
    },
    [Symbol.asyncGenerator]() { return this },
};

If I for-await such a iterator, should we attempt to unwrap the value component on the consumer side?

for await (let value of manualAsyncGen) {
    console.log(value); // a promise or "Hello world"?
}

Related to this, what happens if I for-await over a synchronous iterator which contains a list of promises?

let list = [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)];
for await (let value of list) {
    console.log(value); // A promise or a number?
}

The basic question here is: where should we perform the value unwrapping?

  1. On the producer side (in the yield syntax).
  2. On the consumer side (in for-await and yield *).
  3. Both.

"Both" seems ideal from the point of view of the consumer, but seems excessive in terms of rescheduling continuations on the job queue.

If we do it just on the producer side, then we'll want to create a more complicated sync-to-async iterator conversion algorithm, such that values coming from the sync iterator are property unwrapped.

If we do it just on the consumer side, then users wanting to manually call next, etc, will have to take care to unwrap any promises in the "value" component.

@RangerMauve
Copy link

What about forbidding async iterators that return a promise as the value?

@domenic
Copy link
Member

domenic commented Dec 24, 2015

I don't have strong feelings about this. I see arguments for all three positions. This has been sitting in my inbox for a long time waiting for me to develop strong feelings, and I haven't.

If I had to pick one I would probably pick (2). My gut is that programs with manual next() calls will be rarer than programs with manually-created async iterators.

Related to this, what happens if I for-await over a synchronous iterator which contains a list of promises?

Error, because Array.prototype doesn't contain a [Symbol.asyncIterator]?

@zenparsing
Copy link
Member Author

If I had to pick one I would probably pick (2).

I think we have to unwrap on the producer side. Consider:

async function x() {
  await delay(10000); // 10sec
  console.log("inside x");
}

async function* a() {
  yield x();
  console.log("after x?");
}

async function main() {
  let iter = a();
  await iter.next();
  await iter.next();
}

main();

// Logs:
// "after x?"
// "inside x"

If I yield a promise, I think I'm going to assume that the promise is fulfilled before execution is resumed, regardless of whether the consumer uses next or for-await.

We still might want to unwrap on the consumer side as well, though. Can we conditionally unwrap the value component? In other words, only await if the value component is a "promise" (nominal or has a "then" method)? I know that's verboten for await expressions, but in the case of for-await we've already awaited the IteratorResult object, and there's doesn't seem to be any advantage to always scheduling a job for the fulfillment of the value component as well.

Error, because Array.prototype doesn't contain a [Symbol.asyncIterator]?

We could do that for for-await (although there are some usability advantages to letting for-await work on sync iterators), but what about yield *? Within an async generator function, we want yield * to delegate to other async iterators.

async function* a() {
  yield 1; yield 2; yield 3;
}

async function* b() {
    yield * a();
}

But it seems like it should also be able to delegate to normal iterables as well:

function* a() {
  yield 1; yield 2; yield 3;
}

async function* b() {
    yield * a();
}

That fits with the goal that "adding the async modifier does not change the semantics of a normal generator, other than delivering the results as promises".

@RangerMauve
Copy link

I think that in the context of async generators, it'd definitely be expected that yeild on a promise would await on it internally before yielding.

@eggers
Copy link

eggers commented Dec 29, 2015

@RangerMauve I agree. I wouldn't want to be required to use syntax like yield await asyncFunc()

@benjamingr
Copy link

I'm for "both",

@zenparsing
Copy link
Member Author

@benjamingr I agree. The spec currently says that if the value component of the iteration result is an object with a "then" method, then it is unwrapped by for-await and yield*.

I also have GetAsyncIterator fall back to [Symbol.iterator] if [Symbol.asyncIterator] is not present. Combined with the value unwrapping on the consumer side, this allows things like:

async function f() {
    for await (let x of [promise1, promise2, promise3]) {
        // x is the unwrapped value of the promise
    }
}

// and

async function* g() {
    yield * [promise1, promise2, promise3];
}

Seems pretty intuitive to me. Thoughts?

@benjamingr
Copy link

People often criticize the choice to unwrap promises automatically - but frankly I've been using them and answering peoples' questions about them for a while now, and I'm yet to see a motivating example against unwrapping them. If there are cases where you explicitly don't want to unwrap them you can always wrap them in an object.

I think users would be baffled if they have a for-await loop and they get a promise back. So I'm all for "unwrap on both sides". I don't feel very strongly about it, but I think it would create the least astonishment - especially given the semantics of async/await and of libraries that use yield for coroutines.

@RangerMauve
Copy link

Also agree with "both"

@hayes
Copy link

hayes commented Jan 25, 2016

Does unwrapping work recursively? if you yield a Promise<Promise<Number>> will you be yielding a Number or a Promise<Number> if its the later, unwrapping in both places will produce very unexpected results. I don't think Promises of Promises are very common, but it should be a defined behavior.

@RangerMauve
Copy link

@hayes AFAIK Promises can't resolve to Promises.

@zenparsing
Copy link
Member Author

@hayes We'll do unwrapping in the same way that await does unwrapping.

@hayes
Copy link

hayes commented Jan 25, 2016

thanks for the clarification. I am not as familiar with the spec as I should be. Some older implementations of promises allowed promises to resolve to promises. This makes a lot more sense now.

@zenparsing
Copy link
Member Author

Reminder to myself: there was some contention about this issue, partly because of promise auto-unwrapping in general. I need to go back through this issue and gather arguments.

@zenparsing
Copy link
Member Author

Let's explore this once again. Let's say that we're going to unwrap the value component on the consumer side (in for-await). Should we still unwrap on the producer side as well?

I think the answer is still yes, because users will write yield something() and intend yield await something(), and in the absence of unwrapping on the producer side, these two expression have very different meanings.

async function* agf() {
    // I intended to await the delay, and I'm confused about why
    // the generator can continue before 1 sec has elapsed.
    yield delay("hello", 1000); // 1 sec
    yield delay("world", 1000);  // 1 sec
    yield "stop";
}

let iter = agf();
let timer = Timer.start(); // measure time

// Skip the first two items
iter.next();
iter.next();
iter.next().then(_=> {
    assert(timer.elapsed > 2000); // Assertion unexpectedly fails!
});

If you want to actually yield a promise, then, as @erights has said, you can just box it. That way, we're explicit and there's no confusion.

@waldemarhorwat
@erights

@benjamingr
Copy link

I was under the impression we already discussed this and agreed on "unwrap in both" before - for exactly these problems you describe. It makes the most sense and is the least confusing. Anything else would surprise people using await in regular async functions IMO.

@zenparsing
Copy link
Member Author

@benjamingr We did agree, but it was brought up as a concern in TC39 so I'm digging it back up to solidify the argument (or gather new input).

@erights
Copy link

erights commented Feb 1, 2016

I remain in favor of implicitly collapsing only at the for-await consumer. From https://public.etherpad-mozilla.org/p/tc39-jan-28 , the draft notes from the last TC39 meeting:

MM: (Somewhere in here I made the following unrecorded observation)

There is a third alternative. The async generator can be a generator of anything. "yield" does not imply an implicit await, so no collapsing happens on the generation side. The async iterator that is produced is an iterator of anything. No implicit collapsing within the async iterator itself. Its API remains fully parametric. Rather, the for-await construct does the collapsing, in service of the simple-dominant-use-case consumer, who wants to see an asynchronous sequence of non-promises. Each iteration of the for-await loop does a double await -- one to get the IterationResult, and one to get the promised value.

Any consumer who, instead, wishes to see an asynchronous sequence of anything, can instead write their own loop rather than using the convenient for-await sugar.

Big advantages of not collapsing at the generation side:

  • yielding an unresolved promise does not prevent the generator from proceeding to generate. It is not surprising stalled at the yield point.
  • yielding a rejected promise does not cause a generation-side exception. Rather it simply transmits the rejected promise. A simple for-await consumer is stopped by the rejection throwing the reason.
  • (Pointed out by @dtribble ): An implicit "await" at the "yield" violates the rule that we can find all interleaving points by looking for "await".

@erights
Copy link

erights commented Feb 1, 2016

async function* agf() {
    // I intended to await the delay, and I'm confused about why
    // the generator can continue before 1 sec has elapsed.
    yield delay("hello", 1000); // 1 sec
    yield delay("world", 1000);  // 1 sec
    yield "stop";
}

Because there are no awaits and therefore no interleaving points. One of the most important points you made in your presentation was:

KS: ctrl-f for await, makes it easy to find interleaving points

Your stance here abandons that, as shown by your example.

@zenparsing
Copy link
Member Author

@erights Using my own arguments against me, eh? : )

@benjamingr
Copy link

@erights I'd love to see a use case where any of the advantages of not collapsing is applicable in a real use case.


I think it's not far-fetched people will forget an await before a yield and be surprised that "their async is not working".

Also - ctrl+f await|yield :D

@erights
Copy link

erights commented Feb 2, 2016

No matter what we do, some people will be surprised. I imagine that, if we decide the other way, many will be surprised that yielding an unresolved promise pauses the generator at that point. Worse, attempting to yield a broken promise throws in the generator rather than communicating it to the consumer.

Each side of this has a plausible enough "I am simpler" story that principle of least surprise cuts both ways on this one.

@benjamingr
Copy link

@erights I don't feel strongly about this issue at all - it just sounds weird that people would get raw promises yielded rather than consume state. All I'm asking for is a compelling use case that illustrates the behavior is surprising.

@zenparsing
Copy link
Member Author

An implicit "await" at the "yield" violates the rule that we can find all interleaving points by looking for "await".

Isn't yield itself an interleaving point, with or without await? That is, in a regular async function, we can find all interleaving points by searching for "await", but in async generators we'll need to search for both "yield" and "await", regardless of whether we do implicit unwrapping at yield or not.

Does it change anything if we consider return here?

async function asyncOp1() {
    throw new Error("oops");
}

async function asyncOp2() {
    return asyncOp1();
}

async function* agf() {
    return asyncOp2();
}

let iter = agf();
iter.next().then(resolveHandler, rejectHandler);

Do you think resolveHandler or rejectHandler should be called?

@erights
Copy link

erights commented Feb 2, 2016

resolveHandler. The next() returns a promise for an IterationResult, and the return fulfilled it. The caller of next() must understand the difference between an IterationResult and its payload. The simple consumer, who uses a for-await loop, does not.

@zenparsing
Copy link
Member Author

Out of curiosity, I updated the spec to not implicitly await before yielding and it made the spec-code quite a bit cleaner. Usually a good sign.

@benjamingr
Copy link

To be fair, I don't feel strongly about this. That said, in @zenparsing's example I think most people would expect rejectHandler to be called because an error was thrown.

@meandmycode
Copy link

My 2c on this is that it seems a little usual that a yield would yield into async control flow just because it was within an async generator.

For example, in:

async function* f() {
    yield delay("hello", 1000); // 1 sec
    yield delay("world", 1000);  // 1 sec
    yield "stop";
}

I would hope that even with unwrapping that this is logically very different to:

async function* f() {
    yield await delay("hello", 1000); // 1 sec
    yield await delay("world", 1000);  // 1 sec
    yield "stop";
}

In the first example I would expect all three values to be immediately available to the consumer of iteration, where as the second would yield the first delay immediately, then the second a delay a second later meaning the first example would run in ~1 sec but the second example in ~2 sec.

I feel like I've possibly missed some subtleties in the spec though.

Edit: to clarify, with an for await I would expect the behavior to be consistent (in that it would not attempt to consume the next value until the current value resolved), but not if I manually called next on the iterator.

@zenparsing
Copy link
Member Author

@meandmycode

Edit: to clarify, with an for await I would expect the behavior to be consistent (in that it would not attempt to consume the next value until the current value resolved), but not if I manually called next on the iterator.

I changed the specification to work the way that you describe (i.e. no implicit await when yielding, for-await awaits result.value). So far, in my proof-of-concept code, it seems to work out well.

@eggers
Copy link

eggers commented Feb 18, 2016

@meandmycode just to make it super explicit. You'd expect that if you ran the following code:

let iter = f();
let hello = iter.next().value;
let world = iter.next().value;

console.time('hello');
await hello;
console.timeEnd('hello');

console.time('world');
await world;
console.timeEnd('world');

You'd expect that in the first example, it'd log something like:

hello: 1001.510ms
world: 0.570ms

and the second to output something like:

hello: 1001.510ms
world: 1002.570ms

@domenic
Copy link
Member

domenic commented Sep 17, 2016

Yes and no. It will insert an extra turn or two as some promise wrapping and unwrapping is performed. But from the consumer's point of view it should be identical (mod timing differences).

@domenic
Copy link
Member

domenic commented Sep 17, 2016

Basically yield await Promise.resolve('hello'); is equivalent to

const temp = await Promise.resolve('hello');
// temp contains 'hello'
yield temp;

@hayes
Copy link

hayes commented Sep 17, 2016

This might be a dumb question, but does mean that

yield Promise.resolve(123)
// and
yield await Promise.resolve(123)

Are effectively the same? And if so, what about the yield expressions, are

cont n = yield Promise.resolve(123)
// and
cont n = yield await Promise.resolve(123)

I would expect that in the first case, n would still be a promise. Am I understanding this correctly?

@domenic
Copy link
Member

domenic commented Sep 17, 2016

Yes to the first. To the second, just like in sync generators, n is whatever the caller passed to next(arg). It is not related to what you are yielding, promise or not.

@hayes
Copy link

hayes commented Sep 17, 2016

Right, sorry. I haven't been working with generators in quite a while,
thanks for the clarification.

On Fri, Sep 16, 2016, 5:45 PM Domenic Denicola [email protected]
wrote:

Yes to the first. To the second, just like in sync generators, n is
whatever the caller passed to next(arg). It is not related to what you are
yielding, promise or not.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAx1jwep-BSJvZF6UcEXByxOGWiMFpFiks5qqzhCgaJpZM4G05Mh
.

@zenparsing
Copy link
Member Author

I feel good about this change, but I recall significant opposition. @erights care to comment?

@erights
Copy link

erights commented Sep 17, 2016

Does this reversion to producer side violate the (IMO much more important) contract that interleaving points be syntactically marked by await? If so, please either revert to consumer-side or at least reopen the issue.

@domenic
Copy link
Member

domenic commented Sep 17, 2016

An interleaving point is now possibly marked by yield, yes. I don't intend to reopen; this is the semantics I intend to present to TC39. We can discuss this next week among ourselves or in committee the week after, if you wish.

@domenic
Copy link
Member

domenic commented Sep 17, 2016

(Or here, of course, if you think there is something new to say.)

@benjamingr
Copy link

Honestly I don't think this issue is a big deal. The argument seems mostly theoretical - kind of like the promise recursive resolution argument 3 years ago.

The edge case is mostly theoretical and I suspect the vast majority of users will use async iterators with combinators and for...of.

I say just let the TC vote on this one and do whatever the consensus is.

@zenparsing
Copy link
Member Author

I think unwrapping implicitly on yield is probably fine ("yield" will always be an interleaving point anyway), but are we OK with saying that for-await is non-flattening?

for await (let value of [promise1, promise2]) {
  // is value a promise? 
}

@domenic
Copy link
Member

domenic commented Sep 17, 2016

@zenparsing great example. My thinking, as per the above comments, was that for-await-of should be able to assume that the contract of an async iterable is followed. That's a case where it isn't.

The same problem occurs with yield* [promise1, promise2]. (And occured prior to a996238 as well; yield* has had this problem ever since f03e40e)

To me, these examples show that the sync -> async iterator conversion make it very easy to produce a contract-violating async iterator. That's better than the state before a996238, where simply using yield promise was going to create such a violation, but it's still pretty bad.

Since that conversion is very desirable, I think we just need to make it a bit more complex to handle this. In terms of spec mechanics, this could be specced as creating a second iterator derived from the sync one, or it could be specced via some kind of "conditional double-unwrapping" where we note if the iterator we retrieved was sync, and if so, we double-unwrap at each step. My intuition is that the difference between these two is observable and thus we should go with the simpler/easier to implement, which is the latter.

domenic added a commit that referenced this issue Sep 26, 2016
This fixes the problem discussed in #15 (comment), for both for-await and yield*. It also changes the mechanism by which sync iterator adaptation happens from earlier drafts; before a996238, the adaptation happened in for-await-of by adding an additional await at each step, whereas now, we create an explicit "Async-from-Sync Iterator" adapter. Previously this was discussed around #12 (comment), although there we decided against this option; the recent movements around where unwrapping is performed have made it a better option.
@masaeedu
Copy link

However, with the previous decision in this thread, it is all too easy for the author of an async generator to produce such an instance. All it requires is typing yield promise instead of yield await promise.

In C#, this kind of thing is dealt with using compiler warnings. In JS, it is probably a job for the linter/docs. I don't think it is surprising at all to be able to yield arbitrary values synchronously from a generator, including promises.

@masaeedu
Copy link

From my understanding, a Promise is a duck typed interface, simply requiring an object to have a then method (and possibly some other stuff, been a while). It seems weird to me that any values I yield synchronously may or may not end up travelling through the transpilers async machinery simply because they accidentally happen to fit the shape of a Promise.

@domenic
Copy link
Member

domenic commented Apr 17, 2017

As I noted in the other thread, this decision goes back a long ways, and I don't want to reiterate it here. It seems important to point out, though, that this spec is written for browser implementations, not transpilers, so I'm not sure what your concerns are about "the transpilers async machinery".

@masaeedu
Copy link

@domenic My bad, the "browser's async machinery" then. The point is that I would expect yielding a value synchronously to be syntactically different from awaiting a future, rather than being dependent on the runtime content of the value.

@RangerMauve
Copy link

@masaeedu There's active conversation about this in #93

@domenic
Copy link
Member

domenic commented Apr 17, 2017

Yep, I can understand how someone coming from outside JavaScript might think that. But JavaScript promises have been duck-typed since the beginning, and objects that meet that duck-type are treated specially throughout everything in the language, including the existing await keyword and promise callbacks.

@benjamingr
Copy link

FeIW C# tasks are also duck typed. The compiler checks if the type has a GetAwaiter method in a duck typed way when making the decision. One of the few only duck typed interfaces in C#.

@masaeedu
Copy link

masaeedu commented Apr 17, 2017

@benjamingr The problem is not the duck typing itself, the problem is that the same syntactic construct does totally different things depending on runtime type. You can yield a Task from an async method in C#, it's totally fine. The compiler will just warn you if your method is suspiciously absent of any awaits. The compiler will never transform the rest of your code into a continuation yada yada yada unless you explicitly use await. If I constructed a DLR object that had GetAwaiter and simply returned that from an async function, the runtime wouldn't presume I meant to await it and decide to help me out.

@erights
Copy link

erights commented Apr 17, 2017

@benjamingr "FeIW"? (Googling turned up acronyms that are obviously not what you meant.)

@benjamingr
Copy link

"FWIW" - for what it's worth, just a typo - sorry :)

@benjamingr
Copy link

No matter what we do, some people will be surprised.

So - 5 years later @erights was right and someone was surprised (in StackOverflow) and I revisited this issue 😄

In other news - 5 years later async iterators are omnipresent in Node.js code and pretty popular for browser use cases. So good job everyone working on this for making something useful that creates value for people.

@erights
Copy link

erights commented Jun 14, 2021

Hi @benjamingr this was a long thread. What happened on stackoverflow? Where? What was I right about ;) ?

@benjamingr
Copy link

@erights a user (on Stack Overflow) was surprised that the values of promises were unwrapped when a regular iterable is passed i.e.:

(async () => {
  for await (const foo of [Promise.resolve(1), Promise.resolve(2)]) {
    console.log(foo); // user expected this to be a promise and for it to not "synchronize"
  }
})();

@devsnek
Copy link
Member

devsnek commented Jun 14, 2021

I've also hit this a couple times 😄

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

No branches or pull requests