-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
fs: add disposable mkdtemp #58516
Conversation
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 you'd prefer a new |
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.
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:
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, Unfortunately |
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,
As for this particular method, I think having an awkwardly named |
I personally think dispoable could be a utility, similar with |
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 Also every time I have to use |
I love the idea. |
@himself65 I also thought that '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 I'll get tests up soon exercising at least
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). |
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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Livia Medeiros <livia@cirno.name>
This comment was marked as outdated.
This comment was marked as outdated.
CI failed because
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. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This is ready from my end afaict. Anyone with the merge permission want to push the button? |
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>
Landed in 9523c84 |
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>
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
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
Adds
fs.mkdtempDisposableSync
andfs.promises.mkdtempDisposable
, which work likemkdTemp
except that they return a disposable object which removes the directory on disposal.Fixes #58486.