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

Using with multiple asynchronous resources #16

Closed
benjamingr opened this issue Jul 26, 2018 · 11 comments
Closed

Using with multiple asynchronous resources #16

benjamingr opened this issue Jul 26, 2018 · 11 comments
Labels
for follow-on proposal This issue should be investigated for a follow-on proposal.

Comments

@benjamingr
Copy link

Following a suggestion by @littledan to post feedback here - some things we've noticed building and maintaining Promise.using for bluebird and discussing the topic:

  • Resource acquisition is almost always async
  • Resource acquisition is considerably harder with multiple resources
  • Resource release is sometimes synchronous
  • It is important to deal with the case of failure to acquire one out of many resources

Let's try the absolute simplest case: I want to open two files and copy one to the other. Let's assume an open async method, and a utils.copy async method. Let's assume either opening or writing to the file can throw:

// no exception handling, this is the most basic case
async function copy(first, second) {
  let firstFile = await open(first);
  let otherFile = await open(second);
  await utils.copyTo(first, second);
  await close(firstFile);
  await close(secondFile);
}

Now, let's write it with the syntax from this proposal:

async function copy(first, second) {
  using(let firstFile = await open(first), otherFile = await open(second)) {
    await utils.copyTo(first, second);
  }
}

This is much nicer syntax! but if I understand correctly in this case if the second open rejects the exception is outside the scope of the using (please do correct me if I'm misunderstanding). This means that since the object with Symbol.dispose didn't get to the using yet if one resource was acquired but not yet assigned to the variable declared in that scope.

By the way regarding the prior art: Python solves this with the new-ish async with syntax. I have asked for the help of the proposal author it for discussing the resource management story for Deno and he agreed to help. I can gladly point him here if you'd like.

It is great to see that you have Symbol.asyncDispose. I'm not sure how the fs.promises examples work (does using in an async function implicitly await? fs.promise.open returns a promise and not a resource) but would love to collaborate from the Node.js side in order to help.

@benjamingr
Copy link
Author

Also, huge +1 on pushing this really important idea and concept.

@rbuckton
Copy link
Collaborator

rbuckton commented Aug 3, 2018

[…] if I understand correctly in this case if the second open rejects the exception is outside the scope of the using (please do correct me if I'm misunderstanding). This means that since the object with Symbol.dispose didn't get to the using yet if one resource was acquired but not yet assigned to the variable declared in that scope.

This scenario is called out in the explainer. Each binding in the using statement head is effectively part of a new distinct try/finally-like scope. As a result, the following two examples are semantically equivalent:

async function copy(first, second) {
  using(let firstFile = await open(first), otherFile = await open(second)) {
    await utils.copyTo(first, second);
  }
}

async function copy(first, second) {
  using(let firstFile = await open(first)) {
    using(let otherFile = await open(second)) {
      await utils.copyTo(first, second);
    }
  }
}

@benjamingr
Copy link
Author

Thanks, it wasn't clear that it would work with await this way (it was clear for the synchronous case).

In this case though both files are opened serially which means that if opening a file takes 100ms (just for the example) this example takes 200ms and not 100ms which is what it would take if both would be happening concurrently.

If I'm opening 4 resources - the code would take 4 times as long potentially. Is there any way with this proposal to make the resource acquisition concurrent? That was the part that "bought us" when we added using to bluebird.

@benjamingr
Copy link
Author

Also cc @ralt @spion @petkaantonov

@spion
Copy link

spion commented Aug 3, 2018

IMO its okay for them not to be parallel

You could have a helper function

using (let comp = await composite({res1: awaitable1, res2: awaitable2})) {
  use comp.res1, comp.res2
}

(or another one for arrays)

@benjamingr
Copy link
Author

@spion writing such a composite for the common case for passing two resources to using (they are not dependent on each other) is very challenging.

@littledan
Copy link
Member

Sounds like the proposal handles the use case, and there is nothing to change here, right?

@rbuckton
Copy link
Collaborator

rbuckton commented Aug 4, 2018

I think the concern is that the proposed approach still serializes independent allocations. A Promise.using would basically act like Promise.all, except that if any rejection occurs, all of the resulting values are disposed.

I've also been considering a disposable container (#13 (comment), basically an Array with a @@dispose that calls @@dispose on its elements). If that were the result of a Promise.using call, you could solve parallel resource allocation like this:

using (const [x, y] = await Promise.using([px, py]) {
  
} // x and y are disposed because container is disposed

@benjamingr
Copy link
Author

Precisely @rbuckton, we've found this sort of code to be extremely challenging to write in userland and I think this is something the platform could solve.

If in addition to the using statement the platform provides a way to aggregate resources then this is considered handled. Note that as an alternative it could be concurrent by default:

await using(let x = px, y = py) {

}

This way it is somewhat clearer that the await is on all the statements rather than individual ones. The problem is that I'm afraid with the current proposal people will try to do:

let px = acquireX();
let py = acquireY();
using(const x = await px, y = await py) {

}
// alternative shed color:
using(x, y) await (getX(), getY()) {

}

Which can cause a leak if px acquisition failed since the using is not aware of the fact py acquisition failed.

It is worth mentioning that @spion who was involved in Promise.using in bluebird as much as I was and probably saw as many code bases with it does not consider this to be a big issue.

@benjamingr
Copy link
Author

Also pinging @1st1 who did this (async with) for Python and was very useful in explaining the design before. If I can bug you for more questions - did you have this dilemma about multiple resource acquisition and if so - what did you do? (And if the debate is recorded - may I read it?)

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 7, 2022

The proposal has undergone more than a few changes since this issue was opened. The current proposal has reduced scope to only support synchronous using declarations, though still contains some support for asynchronous disposables.

We plan to introduce some form of asynchronous using in a follow-on proposal once we have worked out a solution for concerns about implicit asynchronous interleaving points raised by @erights. In the mean time, the AsyncDisposableStack class can help to reduce the scaffolding necessary to handle non-trivial asynchronous disposables in userland:

async function f() {
  const stack = new AsyncDisposableStack();
  try {
    const res1 = stack.use(new SomeAsyncDisposableResource());
    const res2 = stack.use(new SomeOtherAsyncDisposableResource());
    ...
  }
  finally {
    await stack.disposeAsync(); // async dispose in reverse order.
  }
}

Unfortunately, the limitation of this approach is that exceptions from async dispose will still shadow any exception from the try block, which is something that would eventually be handled by a future asynchronous using syntax.

I will close this issue for now, and will re-open it or move it when the follow-on proposal for async using has started.

@rbuckton rbuckton closed this as completed Sep 7, 2022
@rbuckton rbuckton added the for follow-on proposal This issue should be investigated for a follow-on proposal. label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for follow-on proposal This issue should be investigated for a follow-on proposal.
Projects
None yet
Development

No branches or pull requests

4 participants