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

Should [[DisposableResourceStack]] be cleared after dispose? #191

Closed
senocular opened this issue Jul 12, 2023 · 9 comments · Fixed by #194
Closed

Should [[DisposableResourceStack]] be cleared after dispose? #191

senocular opened this issue Jul 12, 2023 · 9 comments · Fixed by #194
Assignees
Labels
bug Something isn't working normative Indicates a normative change to the specification
Milestone

Comments

@senocular
Copy link

senocular commented Jul 12, 2023

In 7.5.8 DisposeResources ( disposeCapability, completion ) it loops through [[DisposableResourceStack]] and disposes all of the resources in the stack but I'm not seeing where it might remove those resources or clear the stack. Is there any reason to hold on to them after disposal? If someone is holding on to a DisposableStack after disposal, wouldn't we also want to prevent them from also having to continue to hold on to all of the now disposed resources?

class LongLivedObject {
    #stack = new DisposableStack()
    start() {
      if (this.completed) throw new Error("Completed")
      this.#stack.use(someResource)
      // ...
    }
    stop() {
      // ...
      this.#stack.dispose() // still holding on to someResource?
    }
    get completed() {
      return this.#stack.disposed
    }
}
@ljharb
Copy link
Member

ljharb commented Jul 12, 2023

Hm, that link 404s

@senocular
Copy link
Author

Updated link.

@ljharb
Copy link
Member

ljharb commented Jul 12, 2023

Good call - seems like _disposeCapability_.[[DisposableResourceStack]] should be emptied before the final return

@bakkot
Copy link

bakkot commented Jul 12, 2023

A stack can only be disposed once, and the value of the internal slot is not observable. So there's no reason to empty it out.

@mhofman
Copy link
Member

mhofman commented Jul 13, 2023

@bakkot is right. However I have seen implementation leaks like this one over and over. It's not specific to this case, but I'm wondering if we should have an editor note to point out the content of the stack is no longer observable after disposal.

@senocular
Copy link
Author

Makes sense. I'll close this unless you want to repurpose it for adding a note.

@rbuckton
Copy link
Collaborator

This is a side effect of reusing the same algorithm steps between using/await using and DisposableStack/AsyncDisposableStack. For using/await using, DisposeResources is called just before the environment record is removed, and thus will eventually be GC'd, but a DisposableStack can obviously stick around if there's something holding onto it after dispose() is called. I'm fine with either an explicit spec step to clear out the List, or adding a note.

@rbuckton
Copy link
Collaborator

I am considering adding a note like the one in GeneratorStart indicate that the capability will never be reused, but the step in GeneratorStart is predicated on the [[GeneratorState]] slot being set to completed. A DisposeCapability record doesn't have such a field, so it may actually be better to just set [[DisposableResourceStack]] to an empty List, along with the note.

@rbuckton
Copy link
Collaborator

I am considering a note like this

NOTE: After a DisposeCapability record has been disposed, it will never be used again. The contents of _disposeCapability_.[[DisposableResourceStack]] can be discarded at this point.

However, I'm wondering if it would be better to actually add a [[DisposableState]] slot for pending/disposed and add Asserts in both the AddDisposableResource and DisposeResources AOs to be thorough. I could even remove the [[DisposableState]] slot from DisposableStack and the [[AsyncDisposableState]] slot from AsyncDisposableStack since they would be redundant.

@rbuckton rbuckton added the bug Something isn't working label Jul 19, 2023
@rbuckton rbuckton self-assigned this Jul 19, 2023
@rbuckton rbuckton added this to the Stage 4 milestone Jul 19, 2023
@rbuckton rbuckton added the normative Indicates a normative change to the specification label Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working normative Indicates a normative change to the specification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants