Skip to content

[Fix] DisposeResources: continue chain on throw#10

Merged
ljharb merged 2 commits intoes-shims:mainfrom
ikeyan:fix/async-disposable-defer-throw
Apr 25, 2026
Merged

[Fix] DisposeResources: continue chain on throw#10
ljharb merged 2 commits intoes-shims:mainfrom
ikeyan:fix/async-disposable-defer-throw

Conversation

@ikeyan
Copy link
Copy Markdown
Contributor

@ikeyan ikeyan commented Apr 21, 2026

Fixes #9.

Root cause

In aos/DisposeResources.js, the async path chained each resource's disposal via $then(prev, onFulfilled, rejecter). Two problems:

  1. When one resource's dispose rejected, the next iteration only installed rejecter on the rejected promise, so the earlier-pushed resource's dispose never ran.
  2. rejecter returned undefined, so the final promise resolved to undefined instead of the accumulated CompletionRecord. The caller's completion['?']() then threw the TypeError shown above.

Changes

  • aos/DisposeResources.js: rewrite the async path so every resource's dispose runs regardless of earlier errors, and the promise resolves to the up-to-date completion at the end.
  • aos/AddDisposableResource.js: fix a typo ('SYNC_DISPOSE' -> 'SYNC-DISPOSE') so use(null) / use(undefined) on a sync DisposableStack correctly returns UNUSED (step 1.a) instead of pushing a method-less sync resource. Lands in its own commit with a regression test.
  • test/tests.js: regression tests for the issue (a throwing later-pushed defer runs the earlier-pushed defer, and a promise-rejecting defer surfaces the rejection) and for the SYNC-DISPOSE typo.

The existing behavior of rejecting mixed sync/async stacks is unchanged.

Test plan

  • npm test (runs lint + 822 tape assertions, 812 pre-existing + 10 new) passes.
  • Manual repro from the issue behaves as expected (1 is printed, then Error: 2 is thrown):
~/DisposableStack % cat test.mjs
import './auto.js';

async function main() {
    await using stack = new AsyncDisposableStack()    
    
    stack.defer(() => {
        console.log('1')
    })
    
    stack.defer(() => {
        throw new Error('2')
    })
}

void main()

~/DisposableStack % node test.mjs
1
file:///~/DisposableStack/test.mjs:11
        throw new Error('2')
              ^

Error: 2
    at file:///~/DisposableStack/test.mjs:11:15
    at AsyncDisposableStack.disposeAsync (<anonymous>)
    at main (file:///~/DisposableStack/test.mjs:10:11)
    at file:///~/DisposableStack/test.mjs:15:6
    at ModuleJob.run (node:internal/modules/esm/module_job:430:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:661:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5)

Node.js v24.14.1

@ljharb ljharb force-pushed the fix/async-disposable-defer-throw branch from 2f2219a to 21fd452 Compare April 21, 2026 05:33
Copy link
Copy Markdown
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'm not stoked about the LLM usage here. Either way, the fix (from _ to -) is a good one! Could you please put that, and a regression test, in its own commit, and then put any refactors you're interested in in their own commits?

@ljharb ljharb marked this pull request as draft April 21, 2026 05:34
ikeyan and others added 2 commits April 21, 2026 14:49
Step 1.a of the spec short-circuits null/undefined `V` for sync-dispose,
returning `UNUSED` without pushing a resource. The hint constant elsewhere
is `SYNC-DISPOSE` (hyphen), but the guard here read `SYNC_DISPOSE`
(underscore), so the branch was never taken and `use(null)` / `use()` on a
sync `DisposableStack` pushed a method-less sync resource onto the stack.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…defer

Fixes es-shims#9

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ikeyan ikeyan force-pushed the fix/async-disposable-defer-throw branch from 21fd452 to f749cdc Compare April 21, 2026 05:50
@ikeyan
Copy link
Copy Markdown
Contributor Author

ikeyan commented Apr 21, 2026

The typo fix and minimal fix for #9 with their respective regression tests are in their own commits now.

@ljharb ljharb force-pushed the fix/async-disposable-defer-throw branch from f749cdc to 089d7af Compare April 21, 2026 20:03
@ljharb
Copy link
Copy Markdown
Member

ljharb commented Apr 21, 2026

Thanks. The two fixes look great - the refactors, though, make the AOs deviate farther from the spec text. I try to keep my spec polyfills as close to 1:1 with the spec as possible.

@ikeyan ikeyan force-pushed the fix/async-disposable-defer-throw branch from 089d7af to d533006 Compare April 23, 2026 07:48
@ikeyan
Copy link
Copy Markdown
Contributor Author

ikeyan commented Apr 23, 2026

Removed the refactors from this PR.

@ikeyan ikeyan changed the title [Fix] DisposeResources: continue chain on throw, align with spec [Fix] DisposeResources: continue chain on throw Apr 23, 2026
@ljharb ljharb force-pushed the fix/async-disposable-defer-throw branch from d533006 to 6982b47 Compare April 25, 2026 05:52
@ljharb ljharb marked this pull request as ready for review April 25, 2026 05:52
@ljharb ljharb merged commit 6982b47 into es-shims:main Apr 25, 2026
411 checks passed
@ikeyan ikeyan deleted the fix/async-disposable-defer-throw branch April 25, 2026 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If fn in stack.defer() throws or returns rejected promise, one gets Cannot read properties of undefined (reading '?')

2 participants