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

Implement Symbol.dispose / Symbol.asyncDispose for Deno namespace APIs #20839

Closed
lucacasonato opened this issue Oct 9, 2023 · 5 comments · Fixed by #20845
Closed

Implement Symbol.dispose / Symbol.asyncDispose for Deno namespace APIs #20839

lucacasonato opened this issue Oct 9, 2023 · 5 comments · Fixed by #20845
Labels
feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS runtime Relates to code in the runtime crate
Milestone

Comments

@lucacasonato
Copy link
Member

I've started thinking about how to integrate explicit resource management into Deno APIs. Here is my proposal so far.

  • Deno.FsFile: implement Symbol.dispose that internally calls file.close(), implement Symbol.asyncDispose to just throw
  • Deno.FsWatcher: implement Symbol.dispose that internally calls fsWatcher.close(), implement Symbol.asyncDispose to just throw
  • Deno.HttpConn: implement Symbol.dispose that internally calls httpConn.close(), implement Symbol.asyncDispose to just throw
  • Deno.Server: implement Symbol.asyncDispose to call await server.shutdown(). do not implement Symbol.dispose
  • Deno.stdin / Deno.stderr / Deno.stdout: do not implement disposal methods
  • Deno.Conn: implement Symbol.dispose that internally calls conn.close(), implement Symbol.asyncDispose to just throw
  • Deno.Listener: implement Symbol.dispose that internally calls listener.close(), implement Symbol.asyncDispose to just throw
  • Deno.Process: deprecated, do nothing
  • Deno.ChildProcess: implement Symbol.asyncDispose that internally calls cp.kill(), then await cp.status
  • Deno.Kv: implement Symbol.dispose that internally calls kv.close(), implement Symbol.asyncDispose to just throw

Additionally, we may want to update all of our .unref() methods on resources to return an object with a [Symbol.dispose] method that calls .ref() on the resource (and vice versa).

There is an open question about whether these close methods should be idempotant (not throw if the resource is already closed), or whether it should throw. I am leaning towards them not throwing. This is especially important for resources that may well close through external means, like Deno.ChildProcess. I've asked @rbuckton for guidance on this: tc39/proposal-explicit-resource-management#207

@lucacasonato lucacasonato added public API related to "Deno" namespace in JS needs discussion this topic needs further discussion to determine what action to take runtime Relates to code in the runtime crate feat new feature (which has been agreed to/accepted) labels Oct 9, 2023
@lucacasonato lucacasonato added this to the 1.38 milestone Oct 9, 2023
@lucacasonato
Copy link
Member Author

Ok, open question answered: the dispose methods should not throw if called more than once. This means:

  • for all close(rid) calls, we should use tryClose(rid)
  • for Deno.ChildProcess we should swallow ESRCH errors when calling kill(pid)
  • for Deno.Server we should call server.shutdown(). ensure that shutdown() while we're already shutting down returns a promise that returns when the shutdown is complete, and shutdown() after we've successfully shut down already returns a resolved promise

@lucacasonato
Copy link
Member Author

After some offline discussion, I retract the throwing asyncDispose on everything that only has a sync dispose, it's not helpful and makes testing for disposables more difficult.

@lucacasonato lucacasonato removed the needs discussion this topic needs further discussion to determine what action to take label Oct 9, 2023
@sigmaSd
Copy link
Contributor

sigmaSd commented Oct 9, 2023

  • Deno.makeTempDir: remove the file with Symbol.asyncDispose
  • Deno.makeTempDirSync: remove the file with Symbol.dispose

@sigmaSd
Copy link
Contributor

sigmaSd commented Oct 9, 2023

ah the above returns a string, so it would be a breaking change to modify them

@0f-0b
Copy link
Contributor

0f-0b commented Oct 10, 2023

Symbol.dispose can also be implemented for Datagram, HttpClient and UnsafeCallback.

bartlomieju added a commit that referenced this issue Nov 1, 2023
This commit implements Symbol.dispose and Symbol.asyncDispose for
the relevant resources.

Closes #20839

---------

Signed-off-by: Bartek Iwańczuk <biwanczuk@gmail.com>
Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS runtime Relates to code in the runtime crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants