-
-
Notifications
You must be signed in to change notification settings - Fork 651
feat: Add Symbol.dispose and Symbol.asyncDispose support to DispatcherBase #4258
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests to it?
I think this is a good start. You should make sure tha the result of .compose() can be disposed. Also, docs are missing. |
I'm also confused about what 's after .the close() agent no longer accepts requests, I think we need to completely not accept requests only at .destroy(), or add a method .closeIdleConnections(), which will simply wait for requests to complete and close all connections. |
I agree, I think it should error at this point but not drop any current requests. |
|
||
[Symbol.dispose] () { | ||
this.close(noop) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Symbol.dispose] () { | |
this.close(noop) | |
} |
Queueing a callback does not equate to a synchronous task. The semantics of explicit resource management are such that by the time the disposer returns (or the async disposer's Promise is resolved), the resource disposal should be complete, to maintain the expected order of operations; this is not the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t believe there’s much else we can do for this; the close
method is async by default, we cannot ensure sync guarantees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye, hence only the asyncDispose method makes sense here.
This PR implements the new ECMAScript Explicit Resource Management proposal by adding disposal functionality to the
DispatcherBase
class. The changes include:[Symbol.asyncDispose]()
method that asynchronously closes the dispatcher[Symbol.dispose]()
method that synchronously closes the dispatcherclose()
functionalitynoop
function as a callbackThese changes enable modern resource management patterns:
The implementation follows the TC39 proposal currently in Stage 3 (https://github.com/tc39/proposal-explicit-resource-management)