tc39 / proposal-explicit-resource-management

ECMAScript Explicit Resource Management
https://arai-a.github.io/ecma262-compare/?pr=3000
BSD 3-Clause "New" or "Revised" License
725 stars 28 forks source link

Masking disposable #204

Open jasnell opened 9 months ago

jasnell commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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

mhofman commented 9 months ago

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 commented 9 months ago

You may also wish to take a look at https://github.com/tc39/proposal-explicit-resource-management/issues/195, which suggests a mechanism by which the capability to close a resource could be separated from the resource itself.

jasnell commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

I assume the following is also true...

Correct.

rbuckton commented 9 months ago

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 commented 9 months ago
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 commented 9 months ago

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 commented 9 months ago

@juner:

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

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

jasnell commented 9 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 https://github.com/tc39/proposal-explicit-resource-management/issues/204#issuecomment-1738277085, when the disposer sets this.#foo = undefined.