`AsyncDisposableStack.use()` cannot dispose `Disposable` synchronously

Question

`AsyncDisposableStack.use()` cannot dispose `Disposable` synchronously

Closed this issue 7 months ago · 17 comments

The AsyncDisposableStack.use() is equivalent to await using. But there's no API equivalent to using on AsyncDisposableStack. This can lead to subtle behavior difference if there's async race:

import { describe, expect, test, jest } from "@jest/globals";
require("disposablestack/auto");

function listen(target: EventTarget, event: string, callback: (e: Event) => void): Disposable {
    target.addEventListener(event, callback);
    return { [Symbol.dispose]() { target.removeEventListener(event, callback); } };
}

function race(target: EventTarget, event: Event, resolve: () => void): Disposable {
    return { [Symbol.dispose]() {
        Promise.resolve().then(() => {
            target.dispatchEvent(event);
            resolve();
        });
    } };
}

class MyEvent extends Event {
    constructor(name: string, public n: number) {
        super(name);
    }
}

describe("Async race in dispose", () => {
    test("using", async () => {
        const log = jest.fn();
        await expect(new Promise<void>((resolve) => {
            const target = new EventTarget();
            using listener = listen(target, "event", ({ n }: any) => { log(n) });
            using _ = race(target, new MyEvent("event", 2), resolve);
            target.dispatchEvent(new MyEvent("event", 1));
        })).resolves.not.toThrow();
        expect(log.mock.calls).toStrictEqual([[1]]);
    });

    test("await using", async () => {
        const log = jest.fn();
        await expect(new Promise<void>(async (resolve) => {
            const target = new EventTarget();
            await using listener = listen(target, "event", ({ n }: any) => { log(n) });
            await using _ = race(target, new MyEvent("event", 2), resolve);
            target.dispatchEvent(new MyEvent("event", 1));
        })).resolves.not.toThrow();
        expect(log.mock.calls).toStrictEqual([[1], [2]]);
    });

    test("DisposableStack", async () => {
        const log = jest.fn();
        await expect(new Promise<void>((resolve) => {
            using stack = new DisposableStack();
            const target = new EventTarget();
            stack.use(listen(target, "event", ({ n }: any) => { log(n) }));
            stack.use(race(target, new MyEvent("event", 2), resolve));
            target.dispatchEvent(new MyEvent("event", 1));
        })).resolves.not.toThrow();
        expect(log.mock.calls).toStrictEqual([[1]]);
    });

    test("AsyncDisposableStack", async () => {
        const log = jest.fn();
        await expect(new Promise<void>(async (resolve) => {
            await using stack = new AsyncDisposableStack();
            const target = new EventTarget();
            stack.use(listen(target, "event", ({ n }: any) => { log(n) }));
            stack.use(race(target, new MyEvent("event", 2), resolve));
            target.dispatchEvent(new MyEvent("event", 1));
        })).resolves.not.toThrow();
        expect(log.mock.calls).toStrictEqual([[1], [2]]);
    });
});

Answer 1 · 2023-07-21T07:10:52.000Z

function race(target: EventTarget, event: Event, resolve: () => void): Disposable {
    return { [Symbol.dispose]() {
        Promise.resolve().then(() => {
            target.dispatchEvent(event);
            resolve();
        });
    } };
}

Well there is your problem. If your Disposable is doing async work in its dispose, it really should be an AsyncDisposable. I don't see a way around this.

Answer 2 · 2023-07-21T15:38:43.000Z

function race(target: EventTarget, event: Event, resolve: () => void): Disposable {
    return { [Symbol.dispose]() {
        Promise.resolve().then(() => {
            target.dispatchEvent(event);
            resolve();
        });
    } };
}

Well there is your problem. If your Disposable is doing async work in its dispose, it really should be an AsyncDisposable. I don't see a way around this.

race()'s dispose is not actually doing async work. It returns synchronously. The dispatch event can be a side-effect of disposing race(). In fact the side-effect can happen anywhere before we enter the disposal of listen(), even inside the main function body. I'm just illustrating one case for demonstration purpose.

This can be solved if we provide a synchronous version of stack.use(), so people can choose to use the correct one, just like in an async function you can choose to use using or await using for a Disposable type.

import { describe, expect, test, jest } from "@jest/globals";
require("disposablestack/auto");
const SLOT = require('internal-slot');

class ExtendedAsyncDisposableStack extends AsyncDisposableStack {
    constructor() {
        super();
        SLOT.set(this, '[[DisposableState]]', 'pending');
    }
    useSync(disposable: Disposable): void {
        DisposableStack.prototype.use.call(this, disposable);
    }
    disposeAsync(): Promise<void> {
        SLOT.set(this, '[[DisposableState]]', 'disposed');
        return super.disposeAsync();
    }
    move(): AsyncDisposableStack {
        SLOT.set(this, '[[DisposableState]]', 'disposed');
        return super.move();
    }
}

function listen(target: EventTarget, event: string, callback: (e: Event) => void): Disposable {
    target.addEventListener(event, callback);
    return { [Symbol.dispose]() { target.removeEventListener(event, callback); } };
}

function race(target: EventTarget, event: Event, resolve: () => void): Disposable {
    return { [Symbol.dispose]() {
        Promise.resolve().then(() => {
            target.dispatchEvent(event);
            resolve();
        });
    } };
}

class MyEvent extends Event {
    constructor(name: string, public n: number) {
        super(name);
    }
}

describe("Async race in dispose", () => {
    test("ExtendedAsyncDisposableStack with useSync()", async () => {
        const log = jest.fn();
        await expect(new Promise<void>(async (resolve) => {
            await using stack = new ExtendedAsyncDisposableStack();
            const target = new EventTarget();
            stack.useSync(listen(target, "event", ({ n }: any) => { log(n) }));
            stack.useSync(race(target, new MyEvent("event", 2), resolve));
            target.dispatchEvent(new MyEvent("event", 1));
        })).resolves.not.toThrow();
        expect(log.mock.calls).toStrictEqual([[1]]);
    });

    test("ExtendedAsyncDisposableStack with async use()", async () => {
        const log = jest.fn();
        await expect(new Promise<void>(async (resolve) => {
            await using stack = new ExtendedAsyncDisposableStack();
            const target = new EventTarget();
            stack.use(listen(target, "event", ({ n }: any) => { log(n) }));
            stack.use(race(target, new MyEvent("event", 2), resolve));
            target.dispatchEvent(new MyEvent("event", 1));
        })).resolves.not.toThrow();
        expect(log.mock.calls).toStrictEqual([[1], [2]]);
    });
});

Answer 3 · 2023-07-21T16:19:23.000Z

race()'s dispose is not actually doing async work. It returns synchronously

race's dispose asynchronously dispatches an event, which is a synchronously observable action. The fact it doesn't return a promise to capture that fact is the problem.

Answer 4 · 2023-07-21T16:21:55.000Z

AsyncDisposableStack.prototype.use has the same effects as await using x = y where y is a sync disposable. The await that is implied by await using (and thus by use in this case) was a requirement for consensus.

An AsyncDisposableStack is not intended to emulate the behavior of both async using and using within a single block. You can, however, have an AsyncDisposableStack use a DisposableStack if you want to stripe async and sync resources, i.e.:

using stack1 = new AsyncDisposableStack();

// async resources...
const res1 = stack1.use(getSomeAsyncResource1());
const res2 = stack1.use(getSomeAsyncResource2());

// sync resources...
const stack2 = stack1.use(new DisposableStack());
const res3 = stack2.use(getSomeSyncResource3());
const res4 = stack2.use(getSomeSyncResource4());

// more async resources...
const res5 = stack1.use(getSomeAsyncResource5());
...

or, you use await using for AsyncDisposableStack and using for DisposableStack:

// async resources...
using stack1 = new AsyncDisposableStack();
const res1 = stack1.use(getSomeAsyncResource1());
const res2 = stack1.use(getSomeAsyncResource2());

// sync resources...
using stack2 = new DisposableStack();
const res3 = stack2.use(getSomeSyncResource3());
const res4 = stack2.use(getSomeSyncResource4());

// more async resources...
await using stack3 = new AsyncDisposableStack();
const res5 = stack3.use(getSomeAsyncResource5());
...

Also, as @mhofman said, you are doing an asynchronous thing in dispose, so this isn't a correct usage of dispose. A Symbol.dispose method should do all of its cleanup synchronously. This was actually an issue I noticed in NodeJS as well, as some close methods appear to be synchronous but actually have asynchronous effects that aren't apparent in the API. If a @@dispose has any observable asynchronous effects, it probably should be an @@asyncDispose.

Answer 5 · 2023-07-21T16:37:31.000Z

@mhofman this does beg the question, is it imperative that there is an Await for every individual resource added via await using, even if it exposes an @@dispose method, or just that there is at least one Await at the end of the block? The way the spec text is currently written, there is an implicit await undefined for each sync resource, including null and undefined, but we could just as easily change this to a single await undefined at the end of the block if there were any await using declarations but no await was otherwise triggered.

In essence, given the following:

{
  await using x = null, y = null, z = null;
}
f();

Is a single await undefined at the end of the block before the call to f() sufficient, or do we need to have all three?

Answer 6 · 2023-07-21T16:38:46.000Z

Note that prior to amending the specification text to mandate an implicit await, in the above case no await would have occurred at all for the above case, so the change to enforce an await went from one extreme to the other without considering a potential middle ground.

Answer 7 · 2023-07-21T17:17:19.000Z

@rbuckton
An AsyncDisposableStack is not intended to emulate the behavior of both async using and using within a single block.

Why not? In the implementation for using, a single stack is shared for both using and await using. I don't see why we have to create multiple AsyncDisposableStack and DisposableStack nesting together to achieve the same effect.

Answer 8 · 2023-07-21T17:27:31.000Z

@rbuckton that's a very good point, and one at which I was arriving at myself. Let me think more about this, and double check with @erights, but I think we should be able to make that relaxation.

Answer 9 · 2023-07-21T17:31:26.000Z

Is a single await undefined at the end of the block before the call to f() sufficient, or do we need to have all three?

One is probably fine, but personally I don't think it's worth the additional complexity. await undefined is not particularly expensive, particularly in the context of code which is already actually async.

Answer 10 · 2023-07-21T17:54:30.000Z

We are ok with a specified deterministic collapse of await at the end of block. This would effectively make the number of turns spent at the end of the block dependent on the runtime values of the await using, however it is already the case that this number of turns is influenced by the implementation of each @@asyncDispose method executed.

That said, I do share @bakkot's concern that this would make the spec more complex for reasons I am still unclear about.

Answer 11 · 2023-07-21T19:21:04.000Z

I was looking into this not too long ago. The approach I had considered was to add a slot to DisposeCapability indicating whether there were any await using declarations in the block (regardless as to the actual resource value), merging CreateDisposableResource and GetDisposeMethod such that only resources with @@asyncDispose are added with an async-dispose hint, and modifying DisposeResources to track whether there were any async-dispose resources that are disposed and, if there are none and the DisposeCapability contained any await using declarations then an implicit await undefined would occur. An AsyncDisposableStack wouldn't necessarily need to set the new slot on DisposeCapability since it's @@asyncDispose always returns a Promise anyways.

IMO, that would result in far clearer spec text than the current approach with respect to await using x = null, since that just adds a resource with no value or method just to trigger an await.

Answer 12 · 2024-03-21T19:07:47.000Z

I will have a PR up shortly for the deterministic collapse of Await, though I do have a question for @mhofman and @erights:

Given the following code:

{
  await using y = syncDisposable, x = asyncDisposable;
  happensBefore;
}
happensAfter;

Would you consider a single Await for x's asynchronous disposal to be sufficient for enforcing the invariant that happensAfter occurs in a later turn from happensBefore, or would you expect that happensAfter occurs in a later turn than y's synchronous disposal as well?

I favor the former, in which y's sync dispose and happensAfter occur in the same turn, as it is consistent with the current spec behavior when you mix await using and using in the same block:

{
  using y = syncDisposable;
  await using x = asyncDisposable;
  happensBefore;
}
happensAfter;

Here, happensAfter occurs in a later turn than happensBefore, but the same turn as y's dispose.

I could be convinced otherwise, though, since that does result in a subtle difference vs

{
  await using y = syncDisposable;
  happensBefore;
}
happensAfter;

Since there is no async disposable in the list, we would introduce an implicit Await between y's synchronous dispose and happensAfter.

Answer 13 · 2024-03-21T22:07:01.000Z

I've changed my position here slightly to err on the side of consistency in both directions. Collapse of Await can only occur between synchronous resources in an await using. For example:

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.

I've put up #216 to show how this could work.

Answer 14 · 2024-03-22T11:31:24.000Z

What would happen with the following?

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

My expectation is that HAPPENS_AFTER runs in a new turn, regardless of the nature of the value associated with A.

I will need to think further on other collapsing behavior.

Answer 15 · 2024-03-22T11:39:31.000Z

HAPPENS_AFTER happens in the following turn (unless both B[@@dispose]() and A[@@asyncDispose]() threw synchronously).

I think this particular one is a problem. There is a syntactic await which can result in no new turn. From a static analysis pov, this is not acceptable.

Answer 16 · 2024-03-22T13:14:01.000Z

I think this particular one is a problem. There is a syntactic await which can result in no new turn. From a static analysis pov, this is not acceptable.

This is how await and for await work today. An await must operate on a normal completion. A synchronous throw completion always bypasses the await:

Changing that behavior would be wildly inconsistent with the rest of the language.

Answer 17 · 2024-03-22T13:16:53.000Z

Also, the current proposal text operates in this way, as it aligns with Await handling elsewhere:

(async function () {
    try {
        await using x = { [Symbol.dispose]() { throw "sync"; } };
        Promise.resolve().then(() => { console.log("HAPPENS LATER"); });
        console.log("HAPPENS BEFORE");
    }
    catch {
        console.log("HAPPENS AFTER");
    }
})();

This would also print

HAPPENS BEFORE
HAPPENS AFTER
HAPPENS LATER

Per Dispose:

Which aligns with Evaluation of AwaitExpression:

In both cases we use ReturnIfAbrupt before applying the await, so synchronous throw completions will bypass the Await