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

Masking disposable #204

Open
jasnell opened this issue Sep 27, 2023 · 16 comments
Open

Masking disposable #204

jasnell opened this issue Sep 27, 2023 · 16 comments

Comments

@jasnell
Copy link

jasnell commented Sep 27, 2023

@ljharb... my apologies if this has already been discussed and I just missed it, but what would be expected to happen in the following case:

{
  using foo = bar();
  {
    using baz = foo;
  }
  console.log(foo);
}

As I understand it, the inner block will call the Symbol.dispose on the object instance, correct?

What if that is passed to a function in a library and the caller is not aware that the library is making use of using...?

// library.js
export function handle(xyz) {
  using abc = xyz;
}

// main.js
import { handle } from 'library.js'

{
  using foo = bar();
  handle(foo);
  console.log(foo);
}

Again, as I understand it, when the scope for handle(...) exits the Symbol.dispose will be called.

If that is correct, it may be worthwhile to provide a means of passing a disposable around in a way that masks the fact that it is disposable.

handle(undisposable(foo)); // throws! because `undisposable(foo)` returns an object that proxies `foo` without `Symbol.dispose`.

This is obviously possible to accomplish using a Proxy but if my understanding here is correct, might this be common enough that it is worth baking into the proposal?

@ljharb
Copy link
Member

ljharb commented Sep 27, 2023

I think your analysis is correct, and it wouldn’t be fully achievable through a Proxy because that would break internal slot/private field usage.

Since this proposal is already stage 3 it wouldn’t be appropriate to add anything new to it, but i think the real answer is, if you have a thing with a capability and want to control the use of the capability, you just don’t give the thing to anyone else.

@jasnell
Copy link
Author

jasnell commented Sep 27, 2023

hmm, ok, that constrains a few options I think but it makes sense. Thank you!

@mhofman
Copy link
Member

mhofman commented Sep 27, 2023

IMO, while this proposal is about explicit resource management, it doesn't absolve the program from thinking about what scope owns a resource's lifetime. There is no explicit transfer or share semantics in JS, and this proposal does not introduce any.

As such I think it remains necessary for functions to document whether they keep control or not of their return value, and whether they assume control or not of their arguments.

@bakkot
Copy link

bakkot commented Sep 27, 2023

You may also wish to take a look at #195, which suggests a mechanism by which the capability to close a resource could be separated from the resource itself.

@jasnell
Copy link
Author

jasnell commented Sep 27, 2023

FWIW, the key use case I'm thinking about here are host-defined objects with a wrapping JS object and an internal underlying C++ object. Specifically, can we safely dispose of the underlying C++ object when Symbol.dispose is called without it being... surprising. The short answer is yes, it's all manageable but as you say, we still have to think about what scope owns the lifetime. That doesn't change.

I do think there are some potential footguns buried in here but some well crafted linting rules ought to be able to catch those.

@jasnell
Copy link
Author

jasnell commented Sep 27, 2023

Another quick clarification, I assume the following is also true...

function foo() {
  using xyz = bar();
  return promise.then(() => {
    // xyx captured by closure will have been disposed by this point!
  });
}

@bakkot
Copy link

bakkot commented Sep 27, 2023

I assume the following is also true...

Correct.

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 28, 2023

If your intent is to explicitly give ownership of a resource to another function, one option is to emulate DisposableStack.prototype.move(), in which you create a copy of the object and give it the original object's internal state, then clear out the original object's internal state w/o disposing.

class Foo {
  #state;
  ...
  [Symbol.dispose]() {
    if (this.#state) {
      const state = this.#state;
      this.state = undefined;
      cleanup(state);
    }
  }
  move() {
    const copy = new Foo();
    copy.#state = this.#state;
    this.#state = undefined;
    return copy;
  }
}

function handle(foo) {
  using bar = foo;
}

{
  using foo = new Foo();
  handle(foo.move());
}

// or...
function handle(foo) {
  // some logic that may throw before I call 'move()'...
  using bar = foo.move();
}

{
  using foo = new Foo();
  handle(foo);
}

If your intent is to not provide ownership, you can wrap your resource in a disposable container that is able to perform cleanup on your object:

// Gives `FooContainer` access to private cleanup of `Foo`
let disposeFoo;

class Foo {
  ...
  static { disposeFoo = foo => foo.#disposePrivate(); }
  #disposePrivate() { ... }
}

class FooContainer {
  #foo = new Foo();
  get foo() { return this.#foo; }

  [Symbol.dispose]() {
    if (this.#foo) {
      const foo = this.#foo;
      this.#foo = undefined;
      disposeFoo(foo);
    }
  }
}

{
  using fooContainer = new FooContainer();
  const foo = fooContainer.foo;
  handle(foo);
}

@juner
Copy link

juner commented Sep 28, 2023

function foo() {
  const xyz = bar();
  return promise.then(() => {
    return xyz;
  },(e) => {
    xyz[Symbol.dispose]();
    return Promise.reject(e);
  });
}

@jasnell Shouldn't we leave using to the caller as above...?

@rbuckton
Copy link
Collaborator

rbuckton commented Sep 28, 2023

Another quick clarification, I assume the following is also true...

Yes. using lifetime is scoped to the block. If you wish it to live long enough to be reachable in Promise.prototype.then you have two options:

  1. Switch to async/await:
async function foo() {
  using xyz = bar();
  const result = await promise;
  // xyx has not been disposed.
  return result;
}
  1. Manually handle lifetime, or leverage DisposableStack.prototype.move
function foo() {
  using stack = new DisposableStack();
  const xyz = stack.use(bar());
  ... // any exceptions resulting from acquiring 'promise' result in disposal
  const newStack = stack.move();
  return promise.then(() => {
    // 'stack' has been disposed, but since we moved its contents into 'newStack', 'xyz' has not yet been disposed
  }).finally(() => {
    using stack = newStack; // here we can use 'newStack' to hook back into lifetime management for 'xyz'
    // 'xyz' is now disposed.
  });
}

One of the main purposes of DisposableStack is to give you additional control over lifetime for situations like these, without having to revert back to manual cleanup behavior using try..finally.

@jasnell
Copy link
Author

jasnell commented Sep 28, 2023

@juner:

Shouldn't we leave using to the caller as above...?

Generally, yes, but ... Misbehaving Code :-)

@jasnell
Copy link
Author

jasnell commented Sep 28, 2023

Really appreciate the responses, all super helpful. What I imagine from all of this is likely a new range of linter rules that help catch the problematic patterns and promote these correct approaches.

@guybedford
Copy link

Related to this topic - how does one avoid a double dispose call footgun? Is the expectation that dispose is always singular? Would it ever make sense for dispose to delete its own property (ie, having a call to Symbol.dispose make the object as dead using another symbol, or actually calling delete obj[Symbol.dispose] / obj[Symbol.dispose] = null inside of a dispose call?

@rbuckton
Copy link
Collaborator

I generally wouldn't recommend deleting a method since that changes the object's shape (which can affect runtime performance). It also wouldn't prevent something like:

{
  using a = ...;
  using b = a;
  
} // essentially disposes 'a' twice

because using captures [Symbol.dispose] during initialization.

Generally, it's better to track whether dispose has been called using something like a private field.

@guybedford
Copy link

Good points. There's just a temptation to not need another slot, while I have come across some double call cases (specifically when wanting to support both a finalization registry and explicit resource management together). Perhaps a pattern of object[Symbol.dispose] = () => {}; might not be so bad for dead objects.

@rbuckton
Copy link
Collaborator

In most cases a disposable has at least one field that holds the actual resource to be disposed. You can almost always just use that field as the disposal indicator rather than declaring a new one, which is exactly what I'm showing in the FooContainer example in #204 (comment), when the disposer sets this.#foo = undefined.

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

7 participants