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

dispose should not be a property on the object #160

Open
rbalicki2 opened this issue Jun 16, 2023 · 8 comments
Open

dispose should not be a property on the object #160

rbalicki2 opened this issue Jun 16, 2023 · 8 comments

Comments

@rbalicki2
Copy link

rbalicki2 commented Jun 16, 2023

If dispose is available on the object itself, the following is broken:

{
  using resource = getResource();
  {
    using resource = resource;
  }
  doSomethingWith(resource); // Oops, this receives a disposed resource
}

Sounds silly, right? Who would do that? Well, let's obfuscate this:

function outer() {
  using resource = getResource();
  // or resource.someMethod(); // <- this could also dispose resource :(
  inner(resource);
  doSomethingWith(resource);
}

function inner(defaultResource) {
  const resource = defaultResource ?? getResource();
  using resource;
}

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 an initialQueryReference, and the parent component outlives the child, then once the child unmounts, the parent will have a disposed query reference.

A safe alternative

type ItemCleanupPair<T> = [T, () => void]
declare function getResource(): ItemCleanupPair<MyResource>;

function foo() {
  using someResource = getResource();
  // someResource has type MyResource
  // you can do whatever you want with someResource, but in this block, it is not disposed!
  // Since the cleanup function is inaccessible, only JS can dispose of it at the end of this block!
}

But users can still get around this

Yes, if you want to, you can do:

function foo() {
  const [resource, dispose] = getResource()
  dispose()
  using resource = [resource, dispose];
  // well yeah, this is broken
}

But:

  • this footgun is opt-in. The author of the using block can avoid this if they want to.
  • this is a fundamental limitation of using a language like JavaScript that doesn't track references. /shrug
@bakkot
Copy link

bakkot commented Jun 16, 2023

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 inner consumer could write

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. using is a declaration form; it doesn't introduce a new block.)

@rbalicki2
Copy link
Author

rbalicki2 commented Jun 18, 2023

Apologies about using the wrong syntax, I've updated the original post.

I fundamentally agree with you, but wonder two things:

  • consumers can always do irresponsible things (delete properties, overwrite globals, etc.), but is that a reason to make it easy?
  • In what ways is this less convenient?

Anyway, apologies for not raising objections sooner, as this certainly could've been hashed out in earlier stages.

@bakkot
Copy link

bakkot commented Jun 18, 2023

In what ways is this less convenient?

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 Symbol.dispose alias for that method. With this change, you'd have to change all of your resource-producing APIs to instead vend a [ resource, cleanup ] pair instead, which is a much bigger change and is less convenient to work with for anyone not making use of the new syntax.

@rbuckton
Copy link
Collaborator

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.

@rbalicki2
Copy link
Author

There is also nothing that prevents a resource from being reusable. Connections can be reopened, tracing blocks can be reset, etc.

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.

large number of nursery objects

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 payload) to create the resource. The moment your example needs that, you either recreate the TraceBlock or mutate the existing one and hope that doWork is not called recursively.

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:

const getTraceBlockResource = text => {
  tracingSubsystem.enter(text);
  return [null, () => tracingSubsystem.exit(text)]
}

With this change, you'd have to change all of your resource-producing APIs to instead vend a [ resource, cleanup ] pair instead, which is a much bigger change

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!

@rixtox
Copy link

rixtox commented Jun 21, 2023

A dispose function should be called once and only once. Allowing a Disposable object to be reused like the TraceBlock example, is a kind of abuse. What you should do instead is ref-counting: increment when you enter() and decrement when you exit(), and dispose when the count reaches to 0.

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.

@mayorovp
Copy link

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

@rbuckton
Copy link
Collaborator

large number of nursery objects

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 payload) to create the resource. The moment your example needs that, you either recreate the TraceBlock or mutate the existing one and hope that doWork is not called recursively.

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.

One of my motivations for this proposal is the possibility of leveraging using for several thread-synchronization primitives that may be introduced as part of the Structs and Shared Structs Proposal, whose own goal is improving performance of applications that want to leverage multiple threads. Keeping the disposal mechanism be fairly lightweight is directly in line with the performance goals of that proposal.

It would be better to write the above as:

const getTraceBlockResource = text => {
  tracingSubsystem.enter(text);
  return [null, () => tracingSubsystem.exit(text)]
}

With this change, you'd have to change all of your resource-producing APIs to instead vend a [ resource, cleanup ] pair instead, which is a much bigger change

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!

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 using semantics in #195, with some background in #49 and #159 (though #159 is fairly long so I would recommend starting at #159 (comment)). The gist of that discussion is the potential to add something like a [Symbol.enter]() method to a disposable that could be used to return the actual resource. I think this might achieve what you're looking for, so I would direct you to #195 for further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants
@bakkot @rixtox @rbuckton @rbalicki2 @mayorovp and others