search

tc39 / proposal-explicit-resource-management

ECMAScript Explicit Resource Management
https://arai-a.github.io/ecma262-compare/?pr=3000
BSD 3-Clause "New" or "Revised" License
725 stars 28 forks source link

Fix `DisposeCapability` leak #194

Closed rbuckton closed 2 months ago

rbuckton commented 11 months ago

The DisposeResources AO fails to indicate that the resources in its [[DisposableResourceStack]] slot will no longer be used and can be freed. As a result, a DisposableStack will inadvertently hold resources in memory until the DisposableStack instance is freed, which is not an intended behavior.

This changes DisposeResources to set the [[DisposableResourceStack]] to a new empty list, along with a NOTE indicating the resources can be freed.

A PR against ecma262 is being tracked by https://github.com/rbuckton/ecma262/pull/8.

Fixes #191

/cc: @tc39/ecma262-editors

github-actions[bot] commented 11 months ago

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

rbuckton commented 3 months ago

I've added this for discussion in the April 2024 plenary.

syg commented 3 months ago

Yeah technically there is no future execution which would be able to observe the instance held in [[DisposableResourceStack]] of a disposed stack, so a strict reading of the definition of liveness would make this unnecessary, but I doubt any sane implementation would check whether the stack has already been disposed when doing gc, so making this explicit is better.

I agree with @mhofman's reading. Strictly speaking, slots already don't keep things alive in and of themselves (we don't have reachability, we have liveness via observability).

Making a note doesn't hurt and makes the spec more readable. I'd call this an editorial fix, not a normative one.

littledan commented 2 months ago

+1 to @syg and @mhofman -- this is not a leak currently. If we considered every internal slot which is left with things in it to be a memory leak, we'd have to change tons and tons of stuff in the spec. If we want to give implementations a hint, we could just note at the point where you'd set it to undefined that the contents will never be used again, and they could feel free to set it to undefined.

rbuckton commented 2 months ago

@syg, does the latest commit align with your suggestion?