Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

@@asyncDispose motivation/use cases #4

Open
littledan opened this issue Sep 3, 2022 · 7 comments
Open

@@asyncDispose motivation/use cases #4

littledan opened this issue Sep 3, 2022 · 7 comments

Comments

@littledan
Copy link
Member

I'd like to understand better the motivation for adding @@asyncDispose and AsyncDisposalStack now, in this proposal, as opposed to focusing on just launching disposal and not waiting on it. Apologies, I haven't caught up on all the threads where this has been discussed; feel free to point me to a past explanation if one is already written.

Your section on how web APIs would integrate with disposal is really interesting and provides good motivation, but not to the point where I fully understand the design. It's notable that these examples include a method .close() both in cases that return undefined and cases that return a Promise. I'm wondering, would this be a reasonable pattern to follow for @@dispose, allowing it to return a Promise, rather than using a separate symbol?

In this case, the only difference between using x = y and using (await x = y) { } would be whether we bother waiting on the value returned from @@dispose--in the former case, we're fine to let disposal happen in the background, but the same thing executes. Similarly, for AsyncDisposableStack, the difference would be whether we block on starting one disposal for the next one, or just launch them all in parallel (and maybe this would be a an option to the constructor rather than a separate class).

I'm asking this partly because I'm having trouble understanding when it is important to block control flow on the disposal completing, as opposed to being content having launched it off in the background. Presumably these use cases went into the design of the referenced web APIs, but I don't know where to find more information about that.

(A way to avoid coming to a conclusion on this question would be to just skip @@asyncDispose and AsyncDisposalStack for now and introduce them later, but given the pattern's usage in the web platform, I can sympathize with the impulse for adding it now.)

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 3, 2022

I'd like to understand better the motivation for adding @@asyncDispose and AsyncDisposalStack now, in this proposal, as opposed to focusing on just launching disposal and not waiting on it. Apologies, I haven't caught up on all the threads where this has been discussed; feel free to point me to a past explanation if one is already written.

Launching and not waiting on an asynchronous dispose is extremely unsafe as it can introduce race conditions in asynchronous code. Asynchronous dispose is also necessary for scenarios like three-phase commit transactions, where you need to ensure that the transaction has successfully committed (and all changes flushed to their respective database/system) before continuing. You also would not be able to be properly aware of any exceptions raised, which is treated as a potential crash condition in NodeJS (unhandled rejections). I would strongly advise against a "fire and forget" approach for disposing resources asynchronously. We already have precedent for this as well with for await, Symbol.asyncIterator, and an async iterator's return method, as for await will Await the result of return.

Your section on how web APIs would integrate with disposal is really interesting and provides good motivation, but not to the point where I fully understand the design. It's notable that these examples include a method .close() both in cases that return undefined and cases that return a Promise. I'm wondering, would this be a reasonable pattern to follow for @@dispose, allowing it to return a Promise, rather than using a separate symbol?

The fact that the web APIs are inconsistent between synchronous and asynchronous cleanup in .close() is one of the motivating cases for this proposal. Clearly defining the difference between synchronous and asynchronous dispose and having tools (using, DisposableStack, and AsyncDisposableStack) would be invaluable to having a consistent mechanism for managing resources. I would be strongly against reusing @@dispose for both sync and async dispose.

In this case, the only difference between using x = y and using (await x = y) { } would be whether we bother waiting on the value returned from @@dispose--in the former case, we're fine to let disposal happen in the background, but the same thing executes. Similarly, for AsyncDisposableStack, the difference would be whether we block on starting one disposal for the next one, or just launch them all in parallel (and maybe this would be a an option to the constructor rather than a separate class).

It's almost never a good idea to let explicit disposal happen in the background, especially if such cleanup is critical. As mentioned above, resource disposal should be explicit to avoid race conditions in async code and to properly catch and handle exceptions during cleanup.

I'm asking this partly because I'm having trouble understanding when it is important to block control flow on the disposal completing, as opposed to being content having launched it off in the background. Presumably these use cases went into the design of the referenced web APIs, but I don't know where to find more information about that.

The entire premise of this proposal is to explicitly handle and observe cleanup and any exceptions that occur. I would not be comfortable with weakening this stance.

(A way to avoid coming to a conclusion on this question would be to just skip @@asyncDispose and AsyncDisposalStack for now and introduce them later, but given the pattern's usage in the web platform, I can sympathize with the impulse for adding it now.)

While tempting, I've found that async disposal comes up often enough that I'd rather have something in the MVP to support it. The only reason I'm comfortable with postponing using await to a follow on is that the groundwork would be laid for async disposal through the introduction of @@asyncDispose and AsyncDisposableStack.

@littledan
Copy link
Member Author

Yeah, transaction commit is an especially important use case; thanks for explaining. Do you see the web platform use cases as ones where things will get out of order? They look a bit more like resource cleanup and less likely to cause race conditions, but maybe I'm missing something.

I am wondering, how high do we all think the risk is of starting with synchronous-only disposal (e.g., C++, or maybe earlier states of evolution of C#)? I guess if the lack of async disposal is a big source of bugs, we should be able to see them in these sorts of cases. Is the risk so high that introducing syntax for one and not the other is something to be avoided?

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 7, 2022

Do you see the web platform use cases as ones where things will get out of order? They look a bit more like resource cleanup and less likely to cause race conditions, but maybe I'm missing something.

Resource cleanup can still result in race conditions. Even if that's not a concern, memory pressure can still be an issue, especially on embedded or other resource-limited environments. An app may want to ensure that an expensive resource its holding on to, such as an audio or video clip, has been fully removed from memory before loading another one.

I am wondering, how high do we all think the risk is of starting with synchronous-only disposal (e.g., C++, or maybe earlier states of evolution of C#)? I guess if the lack of async disposal is a big source of bugs, we should be able to see them in these sorts of cases. [...]

I think we will likely end up with two very different syntaxes to support sync and async disposal. I've come to believe that the RAII-style using x = ... declarations are the best approach for sync disposal. However, the I also believe such a declaration is unlikely to address @erights's and others concerns about an unmarked implicit await at the end of a block.

At the end of the day, I think we will probably end up with this:

// sync disposal
{
  using x = ...; 
  ...
} // dispose at end of block

// async disposal
using await (y = ...) {
  ...
} // dispose at end of block

I've considered reintroducing the original using statement as a bridge between the two forms, but I'd still rather have the RAII-style to start.

[...] Is the risk so high that introducing syntax for one and not the other is something to be avoided?

I think both are extremely valuable, but I think that having at least AsyncDisposableStack for async resources is at least a good interim solution, since it reduces try..finally boilerplate and would still be compatible with a future async using follow-on:

async function f() {
  const stack = new AsyncDisposableStack();
  try {
    const ares1 = stack.use(new AsyncResource1());
    const ares2 = stack.use(new AsyncResource2());
    ...
  }
  finally {
    await astack.disposeAsync(); // disposes of ares2 and ares1, but unfortunately suppresses errors from the `try`.
  }
}

@acutmore
Copy link

If a primary motivator for including @@asyncDispose as part of this initial proposal is for its expected use helping to unify the web-APIs, I wonder if there has been any single from web-folk that they would indeed adopt this? If so that may really help solidify the motivation for it not being a separate follow on proposal.

@MadProbe
Copy link

If a primary motivator for including @@asyncDispose as part of this initial proposal is for its expected use helping to unify the web-APIs, I wonder if there has been any single from web-folk that they would indeed adopt this? If so that may really help solidify the motivation for it not being a separate follow on proposal.

As a classical bystander I would say I am strongly against having the async disposal being separated into its own proposal since it significantly footguns the usage of disposables since there are a lot of stuff that would only be usable or feasible to do so if the disposal will be async like file handle close, database transactions, multi-threading with locks, web requests, etc.

@domenic
Copy link
Member

domenic commented Oct 14, 2022

I'm kind of unclear why the web platform (or Node.js) would adopt @@disposeAsync if there's no syntax support for it? Just adding a bunch of aliases/wrappers doesn't really buy much. (But I might not be understanding the proposal; there are a lot of moving parts.)

@rbuckton
Copy link
Collaborator

@domenic Syntactic support for async using is forthcoming, but has been postponed temporarily due to it potentially needing to leverage async do expressions (see #1 for additional context). The proposal should be fairly stable at this point. I don't foresee any major changes to the sync version, and the only change to the async version may be the spelling of the asynchronous block syntax necessary to meet @erights concerns about avoid implicit await points in the language.

Also, I've opened whatwg/html#8557 for discussion about potential WHATWG API integration for either the sync or async APIs.

@rbuckton rbuckton transferred this issue from tc39/proposal-explicit-resource-management Dec 1, 2022
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

5 participants