GeezerVison

I've added an option to the existing mkdtempSync (and will add one to the fs/promises version of mkdtemp) rather than adding a new function. This has the effect of making the return type depend on the options, which is weird to me but follows e.g. the withFileTypes option on readdir.

I've never been a fan of this kind of polymorphic return but won't block on it. Among the key issues is the fact that it is not discoverable. That is, I can't do something like if (fs.makeDisposableTempDir) ... to detect support. But in this case, since using will refuse to work with a noisy runtime error if the user gets it wrong, this should be ok? Still don't like it tho.

If you'd prefer a new mkdtempDisposable method I'm happy to switch to that. I also don't like this kind of polymorphic return (and have argued against it for new JS methods).

Let's see if we can get more folks to comment on it before going through the trouble to change it. @nodejs/collaborators

Own musings:

+1 to the principle, but before starting down this road in earnest, there is probably the opportunity to agree on some kind of design language for "single-use disposables", to keep things consistent across the API rather than drip-feeding a load of independently-designed changes with different semantics.

I'd add a -1 for the polymorphic approach. A "disposable" option on the existing method doesn't just change how the resource is presented (eg. withFileTypes), it completely changes how that returned resource is handled by the user, which feels like more of an existential difference than befits an options parameter. There's also the fact that not all candidate APIs could be modified in this way (eg. eventEmitter.on(), which doesn't have scope to accept an options argument), so this would be committing to at least two different conventions (polymorphism versus separate method).

since using will refuse to work with a noisy runtime error if the user gets it wrong, this should be ok?

This argument might apply here, but it wouldn't necessarily if the same paradigm were applied to other APIs that might return null or undefined.

If we ever get to resurrect #33549, it'd be nice to think of an API design that could be applied to both. I don't have a suggestion, avoiding polymorphism would be great indeed.

Re: design language for single-use disposables, I think that for almost all cases (in node or elsewhere) I'd recommend the following:

• the disposable has a string-named method for cleanup, and Symbol.dispose is an alias for it
• calling the cleanup method again after it has already been called does nothing and does not throw

In most cases it doesn't make sense to put "disposable" in the name of the creation method (and node already has a handful of disposables which are not so named); just call it whatever you'd normally call it. For example, mkstemp could return a disposable.

Unfortunately mkdtempSync returns a string, which cannot be made disposable. That means either doing the polymorphic return that no one is enthusiastic about or adding a new method, and I don't have any good ideas for the method name. Suggestions welcome. Maybe we could say that whateverDisposable() is the way we make a disposable version of primitive-returning APIs? But that doesn't feel great. Which is why I went with the polymorphic return.

That's reasonable, I believe. There are a number of other guidelines we should have, I think.. such as making sure that disposers are always idempotent, and following the conversation we had in the tc39 matrix earlier today, including the assumption that when disposal can be clean (not an error path) or dirty (an exception is thrown and pending), then clean disposal should always be explicit and the code in the disposer should generally assume that there's a pending error. So, for instance,

class MyDisposable {
  doSomething() { /* ... */ }
  close() { /* ... * / }
  abort() { /* ... * / }
  get closed() { /* ... * / }
  [Symbol.dispose]() { if (!this.closed) this.abort() }

  // Or, in the case of something like streams... destroy([error]) where passing an error
  // indicates disposal with a pending exception, while passing nothing indicates clean
  // disposal.
}
{
  using m = new MyDisposable();
  { 
    using n = m;  // causes dispose to be called twice... therefore dispose needs to be idempotent
  }
  m.doSomething();
  m.close();  // explicit clean disposal ... a lot like us requiring that FileHandle is explicitly closed
}

As for this particular method, I think having an awkwardly named mkdtempDisposable is far better than having a polymorphic return. We already have a bunch of awkwardly named API alternatives (mkdtemp, mkdtempSync)... what's a few more?

I personally think dispoable could be a utility, similar with utils.promisify, it could be utils.disposify(entryFn, cleanupFn): () => { resource: ReturnType<entryFn>, cleanup: cleanupFn, [Symbol.dispose]: cleanupFn }

I don't think there's an obvious way to do that here? It's not enough to know that you want to clean up with rmSync; you have to actually specify the parameters (in this case, importantly, recursive: true).

Also every time I have to use promisify I am mad about it because I want to just have the nice things available directly to me instead of having to clobber them together.

I love the idea.
-1 to the polymorphic approach as it might be a mess in TypeScript environments.

@himself65 I also thought that util.disposable() would be a nice thing to have. However, after implementing it, there are some limitations that util.promisify() doesn't have. The issue is that the cleanup function differs from resource to resource. In this PR, for example, the cleanup function needs access to the result of mkdtemp(), as well as process.cwd() at the time of the original call. I think util.disposable() would either end up being too simple to be useful, or full of so many hook points that the complexity isn't worth it.

'use strict';
const { mkdtempSync, rmSync } = require('node:fs');

function disposable(f) {
  return function(...args) {
    const self = this;
    const result = Reflect.apply(f, self, args);

    return {
      result,
      [Symbol.dispose]() {
        Reflect.apply(f[disposable.custom], self, [result, args]);
      },
    };
  };
}
disposable.custom = Symbol('disposable.custom');

mkdtempSync[disposable.custom] = function(path, args) {
  // Note - does not currently handle saving process.cwd().
  rmSync(path, { recursive: true, force: true });
}

const mkdtempSyncDisposable = disposable(mkdtempSync);

{
  using result = mkdtempSyncDisposable('/tmp/foo-');
}

To avoid derailing this specific PR further, I've opened a separate PR that seeks to document guidelines for implementing ERM support to existing APIs. I believe it captures the spirit of what has been discussed here: #58526

OK, switched to fs.mkdtempDisposableSync and fsPromises.mkdtempDisposable.

I'll get tests up soon exercising at least

• directory gets made with the expected prefix
• directory is removed on disposal
  • even if chdir has been called in the mean time
• disposal is idempotent

and anything else people would like to suggest.

Dunno if it's worth copying over the existing tests, of which there are a variety (x, x, etc).

I'll get tests up soon exercising at least (...) and anything else people would like to suggest.

It would be good to add tests to make sure that the dispose function propagates errors during cleanup properly.

Also, it would be good to have test coverage, or at least documentation, for odd edge cases, such as: Is it considered an error if the directory does not exist at the time of disposal? I assume the answer is no to achieve idempotency in some sense. (That also assumes that deletion will be based on the path, not based on the identity of the directory, e.g., using dirfds.)

CI: https://ci.nodejs.org/job/node-test-pull-request/67730/

This is ready from my end afaict. Anyone with the merge permission want to push the button?

Landed in 9523c84

I'm seeing test-wasi fail on Windows with this change on v22.x-staging. If you could open a backport PR, that'd be great

I'm seeing test-wasi fail on Windows with this change on v22.x-staging. If you could open a backport PR, that'd be great

Is this #59146?