-
Notifications
You must be signed in to change notification settings - Fork 32
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
dispose should not be a property on the object #160
Comments
Hm. I'm not the champion, so speaking only for myself - I agree with the general principle (access to a resource shouldn't necessarily imply ability to close the resource), but it's also true that you generally need to trust consumers of your resources not to do dangerous things with them, of which closing them is only one example. And a consumer can choose to handle resources correctly with the current design. In the example you give, for example, the using defaultResource = passedResource == null ? getDefaultResource() : null;
const resource = passedResource ?? defaultResource; and then have the correct "close if not passed" behavior. So while the principle is nice, I'm not sure it warrants the less convenient interface. (Also, sidebar, you have the wrong syntax, which made it hard to follow your examples initially. |
Apologies about using the wrong syntax, I've updated the original post. I fundamentally agree with you, but wonder two things:
Anyway, apologies for not raising objections sooner, as this certainly could've been hashed out in earlier stages. |
Less convenient for producers, I meant. Right now if you're making a resource which needs to be closed, the pattern is to add a string-named method on it - filehandle.close(), etc. Any API which vends such resources can just return that resource. With the current proposal, upgrading such things to be disposable is very simple - you just add a |
There is also nothing that prevents a resource from being reusable. Connections can be reopened, tracing blocks can be reset, etc. Imagine a tracing scenario where you want to trace method calls in a tight loop using a low-overhead tracing mechanism such as ETW (Event Tracing for Windows). Rather than reallocating a tracing block every time the function is called, you could simply reuse an existing tracing block: class TraceBlock {
text;
constructor(text) { this.text = text; }
enter() {
tracingSubsystem.enter(this.text);
return this;
}
[Symbol.dispose]() {
tracingSubsystem.exit(this.text);
}
}
const doWorkTraceBlock = new TraceBlock("doWork");
function doWork(payload) {
using _ = doWorkTraceBlock.enter(); // traces entrance
...
} // traces exit Reusing the same tracing block avoids allocating a large number of nursery objects or potentially needing to wait for GC. |
Yeah, resources can be re-usable. But IMO the proper way to model that is as individually disposable claims to a resource. That is to say, even if the underlying connection is re-openable, a function that receives a disposable claim to a connection should not continue to use it after the claim is closed and should not dispose it twice.
I understand your example, but I guess I think that "most of the time" the resources you care about are not created and released in a hot loop. And "most of the time" you need some parameters (e.g. from An API that helps developers write safe code should prioritize safety, even if it means that the API requires more allocations in a hot loop. It would be better to write the above as:
I agree! My perspective is that it's very unfortunate that many existing APIs are fundamentally broken, and hope that we can break that cycle here! |
A dispose function should be called once and only once. Allowing a Disposable object to be reused like the Exposing the dispose function, or even returning it to the caller can lead to many misuses and abuses. Some people may want to abuse it to do cancellation, some people may make mistakes to excess dispose, some people may forget to dispose, and the list would go on. |
You can easily split resource and disposer with current spec: function openFile(…) {
const file = …;
return {
file,
[Symbol.dispose]() { … },
}
} Meanwhile unchecked arrays are unsafe: using foo = [1,2,3,4,5].map(x => () => console.log(x));
// the line above is clearly erroneous, but the interpreter has no chance of noticing it |
One of my motivations for this proposal is the possibility of leveraging
Using an array for this would not only be poor for performance in a hot loop, but would not be compatible with existing APIs in the DOM and in NodeJS, which would hamper adoption both on the implementer side and on the API consumer side. There is a discussion about a potential future addition to the current |
If
dispose
is available on the object itself, the following is broken:Sounds silly, right? Who would do that? Well, let's obfuscate this:
The way this is currently designed, you cannot pass
resource
anywhere and assume it will not be disposed after the call completes.Sounds silly, who would do that?
Here is an example of where this pattern is used and broken:
https://github.com/facebook/relay/blob/main/packages/react-relay/relay-hooks/useQueryLoader.js#L102-L106 this is the same situation. This
initialQueryReference
is disposed when the component unmounts, so if you have a parent component that renders a child with aninitialQueryReference
, and the parent component outlives the child, then once the child unmounts, the parent will have a disposed query reference.A safe alternative
But users can still get around this
Yes, if you want to, you can do:
But:
The text was updated successfully, but these errors were encountered: