Skip to content

fs: add disposable mkdtemp #58516

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

Closed
wants to merge 17 commits into from
Closed

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented May 30, 2025

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.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 30, 2025
@jasnell
Copy link
Member

jasnell commented May 30, 2025

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
Copy link
Contributor Author

bakkot commented May 30, 2025

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
Copy link
Member

jasnell commented May 30, 2025

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

@Renegade334
Copy link
Contributor

Renegade334 commented May 30, 2025

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
Copy link
Contributor

aduh95 commented May 30, 2025

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
Copy link
Contributor Author

bakkot commented May 31, 2025

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
Copy link
Member

jasnell commented May 31, 2025

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
Copy link
Member

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
Copy link
Contributor Author

bakkot commented May 31, 2025

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
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented May 31, 2025

@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
Copy link
Member

jasnell commented May 31, 2025

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 added 2 commits May 31, 2025 14:37
@bakkot
Copy link
Contributor Author

bakkot commented May 31, 2025

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
Copy link
Member

tniessen commented Jun 5, 2025

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 25, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

Co-authored-by: Livia Medeiros <livia@cirno.name>
@nodejs-github-bot

This comment was marked as outdated.

@bakkot
Copy link
Contributor Author

bakkot commented Jun 28, 2025

CI failed because

  • chdir doesn't work on workers
  • for testing that remove() re-throws errors, the trick I thought I could use to get rmdir to throw on Windows did not in fact work

Hopefully fixed the former by exempting the relevant test from running in workers. For the latter I could not figure out a way to get cleaning up the directory to throw on Windows, so I've just exempted that test from running on Windows. If someone knows of a way I'm happy to update the test. But I think just testing on unix is probably sufficient, since it does demonstrate that the API correctly re-throws those errors.

@nodejs-github-bot

This comment was marked as outdated.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2025
@nodejs-github-bot
Copy link
Collaborator

@bakkot
Copy link
Contributor Author

bakkot commented Jul 6, 2025

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

@richardlau richardlau added semver-minor PRs that contain new features and should be released in the next minor version. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 6, 2025
@jasnell jasnell removed commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 7, 2025
jasnell pushed a commit that referenced this pull request Jul 7, 2025
PR-URL: #58516
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
@jasnell
Copy link
Member

jasnell commented Jul 7, 2025

Landed in 9523c84

@jasnell jasnell closed this Jul 7, 2025
RafaelGSS pushed a commit that referenced this pull request Jul 8, 2025
PR-URL: #58516
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
nodejs-github-bot added a commit that referenced this pull request Jul 8, 2025
Notable changes:

crypto:
  * (SEMVER-MINOR) support outputLength option in crypto.hash for XOF functions (Aditi) #58121
doc:
  * (SEMVER-MINOR) add all watch-mode related flags to node.1 (Dario Piotrowicz) #58719
fs:
  * (SEMVER-MINOR) add disposable mkdtempSync (Kevin Gibbons) #58516
permission:
  * (SEMVER-MINOR) propagate permission model flags on spawn (Rafael Gonzaga) #58853
sqlite:
  * (SEMVER-MINOR) add support for readBigInts option in db connection level (Miguel Marcondes Filho) #58697
src,permission:
  * (SEMVER-MINOR) add support to permission.has(addon) (Rafael Gonzaga) #58951
watch:
  * (SEMVER-MINOR) add `--watch-kill-signal` flag (Dario Piotrowicz) #58719

PR-URL: #58993
RafaelGSS pushed a commit that referenced this pull request Jul 9, 2025
Notable changes:

crypto:
  * (SEMVER-MINOR) support outputLength option in crypto.hash for XOF functions (Aditi) #58121
doc:
  * (SEMVER-MINOR) add all watch-mode related flags to node.1 (Dario Piotrowicz) #58719
fs:
  * (SEMVER-MINOR) add disposable mkdtempSync (Kevin Gibbons) #58516
permission:
  * (SEMVER-MINOR) propagate permission model flags on spawn (Rafael Gonzaga) #58853
sqlite:
  * (SEMVER-MINOR) add support for readBigInts option in db connection level (Miguel Marcondes Filho) #58697
src,permission:
  * (SEMVER-MINOR) add support to permission.has(addon) (Rafael Gonzaga) #58951
watch:
  * (SEMVER-MINOR) add `--watch-kill-signal` flag (Dario Piotrowicz) #58719

PR-URL: #58993
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disposable temporary directory