-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
disposable temporary directory #58486
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
Comments
This is such an interesting feature request. I think I made it work in a "sync fashion" diff --git a/src/node_file.cc b/src/node_file.cc
index ba8a1c464d..8a00502ca8 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -3125,6 +3125,19 @@ static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
if (is_uv_error(result)) {
return;
}
+ auto cleanup = OnScopeLeave([&req_wrap_sync]() {
+ // Delete the folder created by uv_fs_mkdtemp
+ uv_fs_t req;
+ FS_SYNC_TRACE_BEGIN(rmdir);
+ int rmdir_result = uv_fs_rmdir(nullptr, &req, req_wrap_sync.req.path,
+ nullptr);
+ FS_SYNC_TRACE_END(rmdir);
+ if (is_uv_error(rmdir_result)) {
+ // If rmdir fails, we don't throw an error here, but we should log it.
+ fprintf(stderr, "Failed to remove temporary directory: %s\n",
+ uv_strerror(rmdir_result));
+ }
+ });
Local<Value> ret;
if (StringBytes::Encode(isolate, req_wrap_sync.req.path, encoding)
.ToLocal(&ret)) {
It just works, now I have to figure things out for the async way. I don't think we can use the But that is useless. It just creates it and delete it immediately. Not quite sure how we could tie the "parent's context to the callee" |
It's shipping in Chrome, it should be fine to rely on. The only remaining "instability" you should expect is in dumb edge cases like this. |
Then I'm not quite sure if that's something we could backport to previous release lines. |
Well, technically you can because implementing a disposable doesn't require use of any syntax (though you might have to polyfill And if there's a string-named alias (say let tempDir;
try {
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'prefix-'), { disposable: true });
spawnSync(command, [], { cwd: tempDir.path });
// etc
} finally {
tempDir.remove();
} But it is mostly something you'd want to use with the new syntax. |
I think you have a clearer picture about the implementation, would you give it a try? Sadly rn I don't have the bandwidth. |
+1 on this, it's certainly an interesting use case |
Draft PR for discussion: #58516 I had some questions there if anyone wants to take a look. |
Uh oh!
There was an error while loading. Please reload this page.
What is the problem this feature will solve?
I often find myself wanting to make a temporary directory for use during execution of a single function, for example because I'm going to
execSync
some process which creates temporary files in the current directory.mkdtemp
works great to create the thing, but then I have to wrap in in atry
/finally
to handle cleanup.What is the feature you are proposing to solve the problem?
This seems like a good case for the fancy new
using
syntax. It would have to return an object with a[Symbol.dispose]
and apath
property instead of just a string, but then instead ofwe could have
Compare Python's
tempfile
module, which you can use as a context manager for the same sort of behavior:For bonus points it would be nice if this got cleaned up on process exit, but that's more work and it seems fine to just leave "might not get cleaned up if the process exits in the middle of your function" as a documented behavior.
What alternatives have you considered?
We can of course just keep using
try
/finally
, or not cleaning up.The text was updated successfully, but these errors were encountered: