fs: add disposable mkdtemp

[bakkot]

Adds fs.mkdtempDisposableSync and fs.promises.mkdtempDisposable, which work like mkdTemp except that they return a disposable object which removes the directory on disposal.

Fixes #58486.

[jasnell]

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.

[bakkot]

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).

[jasnell]

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

[Renegade334]

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.

[aduh95]

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.

[bakkot]

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.

[jasnell]

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?

[himself65]

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 }

[bakkot]

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.

[ShogunPanda]

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

[cjihrig]

@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-');
}

[jasnell]

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

[bakkot]

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).

[tniessen]

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.)

[bakkot]

Tests added and I believe I've addressed all comments except @legendecas's concern about anonymous objects; PTAL. Testing error propagation requires making the rm -rf call fail, which I know how to do on Unix and I think I know how to do on Windows but I don't have a box to test it on.

Regarding anonymous objects, I'm happy to switch to using a named interface in the docs but will need someone to suggest what that should look like - would that be nested under the mkdtempDisposableSync section or get its own top-level item? Nesting feels better because it keeps related concepts together, but I don't see any examples of that pattern in the docs currently. For the moment I've just cleaned up the docs to match the pattern for returning anonymous objects used in parseArgs.

---

Also, not sure how to address the test failure:

fs.mkdtempDisposableSync was exposed but is neither on the supported list of the permission model nor on the ignore list

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.10%. Comparing base (7622f0d) to head (d216f6d).
Report is 162 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58516      +/-   ##
==========================================
- Coverage   90.23%   90.10%   -0.13%     
==========================================
  Files         635      640       +5     
  Lines      187632   188448     +816     
  Branches    36857    36948      +91     
==========================================
+ Hits       169303   169798     +495     
- Misses      11094    11363     +269     
- Partials     7235     7287      +52     

Files with missing lines	Coverage Δ
lib/fs.js	98.18% <100.00%> (-0.09%)	⬇️
lib/internal/fs/promises.js	98.14% <100.00%> (-0.18%)	⬇️

... and 137 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

Thanks! I feel like the new doc looks better and alleviates the concern around anonymous interfaces. Adding them to the FS Common Objects section could be a good option.

fs.mkdtempDisposableSync was exposed but is neither on the supported list of the permission model nor on the ignore list

A test needs to be added like

node/test/fixtures/permission/fs-write.js

Lines 202 to 213 in 5584cc5

	{
	assert.throws(() => {
	fs.mkdtempSync(path.join(blockedFolder, 'any-folder'));
	},{
	code: 'ERR_ACCESS_DENIED',
	permission: 'FileSystemWrite',
	});
	fs.mkdtemp(path.join(relativeProtectedFolder, 'any-folder'), common.expectsError({
	code: 'ERR_ACCESS_DENIED',
	permission: 'FileSystemWrite',
	}));
	}

and added to the list in https://github.com/nodejs/node/blob/main/test/parallel/test-permission-fs-supported.js.

[bakkot]

Tried to add the permissions test. Turns out that the async version fails with a weird error (it ends up with an uncaught error despite being wrapped). Then I tried to debug it and it turns out that fs.promises.mkdtemp fails in the same way, but which was presumably unnoticed because it is not tested.

So there's a pre-existing issue which I just exposed by adding a new test. I have no idea how to go about fixing that error and in any case it should presumably not happen in this PR. I've created #58747 for the existing bug. Either it needs to get fixed to unblock this or I can remove the async test for now. It does work in the sense of correctly failing to create the directory, it's just that the internal error handling logic is messed up in a way which prevents testing it properly.

[mcollina]

lgtm

I think there are some releated failures.

[bakkot]

I just removed the failing test for now, since it's a pre-existing issue. Can re-land it with a fix to #58747 if anyone figures out what's going on there.

[jasnell]

Consider using a pattern here like what we do with the various pseudo-classes for params/options cases in Web Crypto. https://nodejs.org/docs/latest/api/webcrypto.html#algorithm-parameters ... these aren't actual classes in the code but are documented as such to improve navigation and documentability in the docs. Essentially, while this new method is actually returning an anonymous object, it can still be documented as if it were a named class.

[bakkot]

I can do this, but where should the docs go? fs has a "Common Objects" section but that says

The common objects are shared by all of the file system API variants (promise, callback, and synchronous).

which is not true of these objects - the sync and async versions return objects with a different shape, specific to that function and no other. My inclination would be to make a sub-heading within the fs.mkdtempDisposableSync and fsPromises.mkdtempDisposable sections, but that's not really how any of the other docs look.

[jasnell]

I would just put them there in the Common Objects section. We can shift things around later if necessary.

[jasnell]

But really it's up to you. Wherever you feel most comfortable with it.

[jasnell]

This really should be expanded to indicate what the encoding is used for. This might be an existing problem in the documentation.

[legendecas]

LGTM but I'd prefer if @jasnell's comment at #58516 (comment) can be incorporated as well.

[bakkot]

Turns out I don't know how to add classes to the docs. I tried but the build failed with an error I don't know how to fix (I'm guessing I'd have to edit type-parser.mjs?), so I reverted it.

I'm personally happy with the current state with anonymous objects in the docs. If someone who prefers named interfaces wants to go through the effort of updating the docs correctly, go for it; you should be able to push to this branch. Otherwise I'll leave it as is.

[jasnell]

That's fine too I think.

[nodejs-github-bot]

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