| 00:14 | <Justin Ridgewell> | We have a 262 issue for that |
| 00:14 | <Justin Ridgewell> | https://github.com/tc39/ecma262/issues/1849 |
| 00:17 | <Mathieu Hofman> | Looks like you and I arrived at the same conclusion |
| 00:18 | <Justin Ridgewell> | Lol, yah down to the step number |
| 00:22 | <Mathieu Hofman> | Actually rejecting the capability wouldn't close the sync iterator, would it? That's the problem today, the rejection flows into the promise capability, and into the consumer of the async iterator, which then quarantines the async iterator |
| 00:59 | <Justin Ridgewell> | It should, given a throw will close via the IfAbruptRejectPromise, which just calls capability.[[Reject]] |
| 01:02 | <Mathieu Hofman> | I don't see it |
| 01:07 | <Mathieu Hofman> | I don't see anything that will close the syncIterator if the promiseCapability is rejected. For that matter, the syncIterator is not given to AsyncFromSyncIteratorContinuation, so it wouldn't have a way to do it. |
| 01:09 | <Mathieu Hofman> | it'd have to be plumbed through as an optional 3rd argument in the case of next, as return and throw already close the syncIterator by definition |
| 01:16 | <Justin Ridgewell> | Let me try digging in again |
| 01:21 | <Justin Ridgewell> | https://tc39.es/ecma262/#sec-%asyncfromsynciteratorprototype%.next |
| 01:21 | <Justin Ridgewell> | Step 6.a gets the sync.next() value |
| 01:22 | <Mathieu Hofman> | it gets the result, aka {value: Promise, done: boolean} |
| 01:22 | <Justin Ridgewell> | Which gets us an { value, done } value |
| 01:22 | <Justin Ridgewell> | Yah |
| 01:23 | <Justin Ridgewell> | Now, done doesn't really matter |
| 01:23 | <Justin Ridgewell> | But we get the value |
| 01:23 | <Justin Ridgewell> | In https://tc39.es/ecma262/#sec-asyncfromsynciteratorcontinuation |
| 01:24 | <Justin Ridgewell> | The promise itself is received as a normal completion, but it's either rejected or will reject |
| 01:24 | <Mathieu Hofman> | and from what I understand AsyncFromSyncIteratorContinuation grabs done and value from the result, awaits the value and resolves a promise with {done, value: awaitedValue} |
| 01:25 | <Justin Ridgewell> | If we pass capibility.[[Reject]] to PerformPromiseThen, then it'll eventually call that reject |
| 01:25 | <Mathieu Hofman> | if the await throws, it rejects the promise, but the syncIterator is not touched anymore |
| 01:25 | <Justin Ridgewell> | There's a close step |
| 01:26 | <Mathieu Hofman> | right but rejecting the promise does nothing besides giving a rejected promise to the async iterator consumer |
| 01:27 | <bakkot> | one fix to this would be to add a handler to valueWrapper which closes the iterator, though that seems kinda silly |
| 01:27 | <Mathieu Hofman> | aye that's what I'm arguing we need to do |
| 01:27 | <bakkot> | another fix is to have for-await-of explicitly coordinate with the wrapper, which seems kinda painful |
| 01:27 | <Mathieu Hofman> | why is it silly ? |
| 01:28 | <Mathieu Hofman> | what we have right now is basically the wrapper not cleaning up after itself |
| 01:28 | <bakkot> | because we'd be unwrapping the promise twice |
| 01:28 | <Mathieu Hofman> | it missed a try-catch when awaiting the value |
| 01:29 | <Mathieu Hofman> | how so, the 3rd argument in step 10 does nothing right now, instead it needs to cleanup and passthrough the error |
| 01:29 | <bakkot> | yeah, that's fair |
| 01:29 | <bakkot> | not really unwrapping it twice so much as handling both branches |
| 01:29 | <bakkot> | sgtm, needs-consensus PRs welcome |
| 01:32 | <Justin Ridgewell> | It's up the chain a bit |
| 01:32 | <Justin Ridgewell> | https://tc39.es/ecma262/#sec-runtime-semantics-forin-div-ofbodyevaluation-lhs-stmt-iterator-lhskind-labelset |
| 01:33 | <Justin Ridgewell> | This is the thing that runs for await (…) |
| 01:34 | <Justin Ridgewell> | For us, iteratorRecord is the wrapped asyncSync iterable, iterationKind is iterate, and iteratorKind is async |
| 01:35 | <Justin Ridgewell> | Step 6.b says "If iteratorKind is async, set nextResult to ? Await(nextResult)." |
| 01:35 | <bakkot> | ForIn/OfBodyEvaluation isn't really in a position to handle the issue, because it can't tell the difference between an async iterator which threw and a sync iterator which returned a rejected promise |
| 01:35 | <Mathieu Hofman> | Step 6.
There is no cleanup of the iterator |
| 01:35 | <bakkot> | it's the wrapper which has to deal with it |
| 01:36 | <Justin Ridgewell> | We can't make those different cases |
| 01:36 | <bakkot> | Which? |
| 01:36 | <bakkot> | I agree we can't make ForIn/OfBodyEvaluation differentiate |
| 01:36 | <Justin Ridgewell> | https://github.com/tc39/ecma262/issues/1849#issuecomment-584961565 |
| 01:37 | <Justin Ridgewell> | They should both be calling the cleanup function |
| 01:37 | <bakkot> | no, neither of them should be |
| 01:37 | <bakkot> | the wrapper should do be calling cleanup when its underlying iterator returns a promise which rejects |
| 01:37 | <Mathieu Hofman> | iterator throw / reject on next exits the iteration abruptly |
| 01:37 | <bakkot> | because at that point it's going to stop iterating the underlying iterator |
| 01:38 | <bakkot> | and when you stop iterating over a well-behaved iterator, you close it |
| 01:38 | <bakkot> | cf https://github.com/tc39/rationale/issues/2 |
| 01:38 | <bakkot> | from ForIn/OfBodyEvaluation's point of view, the thing being iterated is not well-behaved, because its next method returned a promise which rejects |
| 01:39 | <bakkot> | so ForIn/OfBodyEvaluation should not be doing any cleanup |
| 01:39 | <Justin Ridgewell> | I disagree |
| 01:39 | <bakkot> | Which with part? |
| 01:39 | <Justin Ridgewell> | Non-well behaved iterators should cleanup |
| 01:39 | <bakkot> | Well, they don't, anywhere |
| 01:39 | <Justin Ridgewell> | If my iterator throws, it needs to clean up |
| 01:40 | <bakkot> | if your iterator throws, it can do the cleanup itself |
| 01:40 | <bakkot> | we only ever call IteratorClose when we stop iterating for reasons which the iterator could not have known |
| 01:40 | <bakkot> | e.g. breaking out of a loop, etc |
| 01:41 | <Mathieu Hofman> | yeah even I was surprised at first, I agree the behavior on broken iterators make sense, and it'd be impossible to change it. Here however it's the wrapper that's not well behaved by not cleaning up when the consumer would think the wrapper itself is misbehaved for throwing |
| 01:41 | <bakkot> | if the iterator itself is saying that it is done, then it's the iterator's responsibility to do the cleanup |
| 01:42 | <Justin Ridgewell> | Ok, I think we're talking about two separate things now |
| 01:42 | <Justin Ridgewell> | The async wrapper is stopping iteration |
| 01:43 | <Justin Ridgewell> | The underlying sync iterator didn't do anything wrong |
| 01:43 | <Justin Ridgewell> | It can either be left open, or we can clean it up. |
| 01:43 | <bakkot> | Right, my position is that the wrapper should clean up the sync iterator at that point |
| 01:44 | <Mathieu Hofman> | the async wrapper is returning a rejection, which is construed by the consumer as misbehaving. It should know it won't be called again, so it should close its sync source |
| 01:44 | <Justin Ridgewell> | Ok |
| 01:44 | <bakkot> | i.e. we should add a handler to the third argument in step 10 of AsyncFromSyncIteratorContinuation which closes the sync iterator |
| 01:44 | <Justin Ridgewell> | Lol, I think that's what I suggested |
| 01:44 | <bakkot> | hah, well |
| 01:45 | <bakkot> | I am glad we are agreement then, even if violent |
| 01:45 | <Mathieu Hofman> | what you suggested is forwarding the rejection, which already happens. The wrapper needs to explicitly close the sync iterator it holds |
| 01:45 | <bakkot> | oh, yeah |
| 01:46 | <Mathieu Hofman> | admittedly the flow inversion in iterators is a bit mindbending |
| 03:12 | <Mathieu Hofman> | Alright, since this is my first spec PR, can someone take a preliminary look and tell me if that makes sense before I mark it as ready? bakkot maybe? https://github.com/tc39/ecma262/pull/2600 |
| 03:40 | <bakkot> | lgtm |
| 03:44 | <bakkot> | note that you (or someone) will need to present at plenary and get consensus for it to land |