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

Reduce unnecessary Awaits in blocks containing await using #216

Closed
wants to merge 1 commit into from

Conversation

rbuckton
Copy link
Collaborator

@rbuckton rbuckton commented Mar 21, 2024

Per #196 (comment), this reduces the number of Awaits in blocks containing await using and in AsyncDisposableStack to only those necessary to meet the requirement that exiting a block in which an await using is evaluated should always Await at least once, even if the resource defined by await using is null, undefined, or synchronous disposable (i.e., an object with a @@dispose instead of an @@asyncDispose).

As currently specified, the following code results in three Awaits when only one is necessary:

{
  await using x = null, y = null, z = { [Symbol.dispose]() {} };
}

This was also the case in an AsyncDisposableStack containing synchronous disposables:

const stack = new AsyncDisposableStack();
stack.use({ [Symbol.dispose]() { } });
stack.use({ [Symbol.dispose]() { } });
stack.use({ [Symbol.dispose]() { } });
await stack.disposeAsync(); // Awaits four times, including the explicit await on this line.

This PR changes this behavior such that we only need an implicit Await in the following cases:

  • Disposal of an async resource1, so long as it does not throw synchronously.
  • When the disposal of an async resource or async-from-sync resource2 throws synchronously after a preceding async-from-sync resource disposed normally.
  • When transitioning from the normal disposal of an async-from-sync resource declared in an await using, to the disposal of a sync resource3 declared in a using.
  • When the normal disposal of an async-from-sync resource is the last disposal performed for a scope.

Example 1

try {
  using A = syncRes;
  await using B = asyncRes;
  HAPPENS_BEFORE
} catch { }
HAPPENS_AFTER

Here, B has an implicit Await for its disposal, which means (per current proposal text):

  • B[@@asyncDispose]() is invoked in the same turn as HAPPENS_BEFORE.
  • A[@@dispose]() is invoked in the following turn (unless B[@@asyncDispose]() threw synchronously).
  • HAPPENS_AFTER happens in the same turn as A[@@dispose]().

Example 2

try {
  await using A = syncRes, B = asyncRes;
  HAPPENS_BEFORE
} catch { }
HAPPENS_AFTER

Here, A and B both have implicit Awaits, which means (per current proposal text):

  • B[@@asyncDispose]() is invoked in the same turn as HAPPENS_BEFORE.
  • A[@@dispose]() is invoked in the following turn (unless B[@@asyncDispose]() threw synchronously).
  • HAPPENS_AFTER happens in the following turn (unless A[@@dispose]() threw synchronously).

Example 3

try {
  await using A = asyncRes, B = syncRes;
  HAPPENS_BEFORE
} catch { }
HAPPENS_AFTER

Here, only A needs to have an implicit Await, since B will dispose synchronously, which means (proposed change):

  • B[@@dispose]() is invoked in the same turn as HAPPENS_BEFORE.
  • A[@@asyncDispose]() is invoked in the same turn as B[@@dispose]().
  • HAPPENS_AFTER happens in the following turn (unless both B[@@dispose]() and A[@@asyncDispose]() threw synchronously).

Note that this means that the above could all occur synchronously if both B[@@dispose]() and A[@@asyncDispose]() were to throw synchronously, though that is consistent with await in general. However, if A[@@asyncDispose]() were to throw synchronously but B[@@dispose]() were to complete synchronously we would need to introduce an implicit Await to ensure we account for the successful disposal.

This PR also removes unnecessary Awaits from AsyncDisposableStack.prototype.disposeAsync() since you must either await or invoke .then() on the result to observe its effects.

Fixes #196
Fixes #208

Footnotes

  1. An async resource is an object with an @@asyncDispose method initialized in an await using declaration.

  2. An sync-from-sync resource is either null, undefined, or an object with a @@dispose method (but not an @@asyncDispose method) initialized in an await using declaration.

  3. A sync resource is either null, undefined, or an object with a @@dispose method initialized in a using declaration.

Copy link

A preview of this PR can be found at https://tc39.es/proposal-explicit-resource-management/pr/216.

@rbuckton rbuckton added needs-consensus A pull request that needs consensus at TC39 plenary normative Indicates a normative change to the specification labels Mar 21, 2024
@bakkot
Copy link

bakkot commented Mar 21, 2024

Hm. I would lightly prefer to always await for sync-dispose, and only skip awaits (when possible) in the case of await using x = null specifically. Having things happening in the same turn when using await using seems surprising. Nothing happens for the null case so eliding that await is fine.

Copy link

@bakkot bakkot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does what it says, though I'm not totally convinced by the semantics as mentioned above.

@rbuckton
Copy link
Collaborator Author

I can be convinced to only skip for null, it's still an improvement over the current approach.

@mhofman
Copy link
Member

mhofman commented Mar 22, 2024

As I mentioned on the issue, skipping new turn altogether in exceptional cases is not acceptable. The rule that should be respected is that if there is an await evaluated in the block, expressions following the block must execute in a future turn.

@rbuckton
Copy link
Collaborator Author

rbuckton commented Mar 22, 2024

As I mentioned on the issue, skipping new turn altogether in exceptional cases is not acceptable. The rule that should be respected is that if there is an await evaluated in the block, expressions following the block must execute in a future turn.

It should be clarified that must execute is too strong here. If all disposals throw synchronously and the surrounding block is contained within a try-catch block, the catch clause and statements that follow will still execute in the same turn, which is consistent with await elsewhere:

try {
  const fail = () => { throw "sync throw" };
  
  HAPPENS_BEFORE
  
  await fail();
} 
catch {
  HAPPENS_AFTER
}

Even with the await, both HAPPENS_BEFORE and HAPPENS_AFTER will happen in the same turn.

Skipping the new turn in exceptional cases is directly in line with existing ECMASCript. await must operate on a normal completion, a synchronous throw completion always bypasses the await.

@rbuckton
Copy link
Collaborator Author

rbuckton commented Mar 22, 2024

Also, the current specification text skips new turns for synchronous exceptions, as do await and for await:

image

If that were not the case, HAPPENS LATER would be logged before HAPPENS AFTER as it is scheduled before the await.

This does, however, ensure that an await happens if we transition from a collapsed set of async-from-sync operations that succeeded to a synchronous operation such as a using resource disposal, or a sync exception from any other disposal in the scope.

@mhofman
Copy link
Member

mhofman commented Mar 22, 2024

If all disposals throw synchronously and the surrounding block is contained within a try-catch block, the catch clause and statements that follow will still execute in the same turn, which is consistent with await elsewhere:

I don't believe this is equivalent. In the await using case the right hand side of the await evaluated.

To be more clear, if execution proceeded past the await, I believe in both cases the author should be able to assume an interleaving point happens.

@rbuckton
Copy link
Collaborator Author

In the await using case, what is awaited isn't the RHS of the assignment, it's the subscription to the dispose that occurs at the end of the block (i.e., you are awaiting the effects of the using). If there is no other await in the block, when the dispose call is evaluated you are still in the same turn as the block itself. If the dispose throws synchronously, you have not actually reached the point in execution where the await occurs.

The closest example of this is for await, which also has this behavior:

image

In this example, the synchronous throw in next results in execution continuing synchronously out of the loop to the catch clause, thus HAPPENS_BEFORE and HAPPENS_AFTER occur in the same turn. Yes, there is an interleaving point before this, but even that can result in synchronous execution, though the body of the loop isn't evaluated:

image

So this is completely in line with the behavior of both await and for await as it exists in the language today. The only time there is an actual call to Await is if the RHS does not throw synchronously. I believe we must maintain that invariant.

That said, the most likely implementation of a normal @@asyncDispose will be with an async function which will not throw synchronously. The issues lies with how @@dispose is handled in await using as we do not wrap it in a PromiseCapability, we merely Await(*undefined*) if it succeeds. We could do the same thing we do for %AsyncFromSyncIteratorPrototype%.next and wrap it in a PromiseCapability so that sync exceptions turn into rejections, though that would mean we can only collapse Await for null/undefined, which I would be fine with.

Even with that though, a hand-written @@asyncDispose can still throw synchronously just like a hand-written AsyncIterator.next (or AsyncIterator.return) can in a for..await. It's a rare corner case, but I would rather not break with for..await on this.

@rbuckton
Copy link
Collaborator Author

As an alternative to this PR, I've posted #218 to add a PromiseCapability to the @@dispose wrapper and #219 to only perform deterministic collapse on null/undefined resources.

@rbuckton rbuckton added the enhancement New feature or request label Mar 22, 2024
@rbuckton
Copy link
Collaborator Author

Closing this in favor of #218/#219.

@rbuckton rbuckton closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-consensus A pull request that needs consensus at TC39 plenary normative Indicates a normative change to the specification
Projects
None yet
3 participants