fs: add mkstemp functions

[targos]

• fs.mkstemp and fs.mkstempSync return path and fd.
• fsPromises.mkstemp returns path and handle.

@nodejs/fs

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[targos]

/cc @addaleax

[vdeturckheim]

LGTM

[nodejs-github-bot]

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

[benjamingr]

I think a disposer API for the promises side would have been nicer. I maintain tmp-promise and in my experience that's the more popular API.

I'd also steal some tests from tmp but that can be in a follow up PR :]

[targos]

I think a disposer API for the promises side would have been nicer. I maintain tmp-promise and in my experience that's the more popular API.

I'd also steal some tests from tmp but that can be in a follow up PR :]

I don't understand what you are suggesting, sorry.

[benjamingr]

@targos in tmp-promise there are two APIs, one is similar to the one outlined here and the other looks a bit like

fs.promises.withMkstemp(async(file)=>{// when the promise returned from this async function settles, file is automatically closed});

This makes for better error handling most of the time. I am not saying we should ship this sort of API or that if we do we shouldn't ship the one already in this PR - only that in my (anecdotal) experience as a maintainer of tmp-promise it's the more popular one.

[targos]

@nodejs/platform-aix PTAL

[richardlau]

I can take a look on Monday but my suspicion would be that the value of the template (i.e. req->path) passed into the mkstemp C call on completion is undefined in the error case.

[bnoordhuis]

LGTM. The AIX failure looks like it's happening on the libuv or libc level. Needs further investigation but I don't think it's caused by this PR.

[bnoordhuis]

That sounds plausible. POSIX allows it to be unspecified. I'll open a libuv PR to clobber req->path on error to flush out such applications bugs.

[richardlau]

I can take a look on Monday but my suspicion would be that the value of the template (i.e. req->path) passed into the mkstemp C call on completion is undefined in the error case.

Yes, looks like that's the case here:

diff --git a/deps/uv/src/unix/fs.c b/deps/uv/src/unix/fs.c
index f5b2b94207..ce9683cb51 100644
--- a/deps/uv/src/unix/fs.c+++ b/deps/uv/src/unix/fs.c@@ -336,7 +336,9 @@ static int uv__fs_mkstemp(uv_fs_t* req) {
if (req->cb != NULL)
uv_rwlock_rdlock(&req->loop->cloexec_lock);
+printf("before mkstemp, path: %s\n", path);
r = mkstemp(path);
+printf("after mkstemp, path: %s\n", path);
/* In case of failure `uv__cloexec` will leave error in `errno`,
* so it is enough to just set `r` to `-1`.

bash-4.4$ ./tools/test.py parallel/test-fs-error-messages
=== release test-fs-error-messages ===
Path: parallel/test-fs-error-messages
before mkstemp, path: /home/riclau/sandbox/github/nodejs/test/.tmp.0/non-existent/foo/barXXXXXX
after mkstemp, path:
before mkstemp, path: /home/riclau/sandbox/github/nodejs/test/.tmp.0/non-existent/foo/barXXXXXX
after mkstemp, path:
(node:13172966) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)
assert.js:920
throw err;
^
AssertionError [ERR_ASSERTION]: The input did not match the regular expression /^\/home\/riclau\/sandbox\/github\/nodejs\/test\/\.tmp\.0\/non\-existent\/foo\/bar/. Input:
''
at validateError (/home/riclau/sandbox/github/nodejs/test/parallel/test-fs-error-messages.js:670:12)
at /home/riclau/sandbox/github/nodejs/test/common/index.js:363:15
at fs.js:163:23
at FSReqCallback.oncomplete (fs.js:1893:14) {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: '',
expected: /^\/home\/riclau\/sandbox\/github\/nodejs\/test\/\.tmp\.0\/non\-existent\/foo\/bar/,
operator: 'match'
}
Command: out/Release/node --expose-internals /home/riclau/sandbox/github/nodejs/test/parallel/test-fs-error-messages.js
[00:00|% 100|+ 0|- 1]: Done
bash-4.4$

POSIX doesn't specify what happens to the template string in the error case: https://pubs.opengroup.org/onlinepubs/9699919799/functions/mkdtemp.html

The Linux documentation also points out that the template may be undefined for some errors (EEXIST): https://www.man7.org/linux/man-pages/man3/mkstemp.3.html

So it's not safe to assume the contents of the template (req->path) if the mkstemp call fails as the test is currently doing. If we deem it useful from a debugging/errors point of view in Node.js we should probably store a copy of the template before making the call into libuv.

[targos]

@bnoordhuis I'm not sure anymore. Is this blocked on work that should be done on the libuv side, or should I do something here to fix the issue?

[bnoordhuis]

@targos There's still some work to be done in libuv but nothing critical, it doesn't have to hold up this PR. I'd go with Richard's suggestion:

we should probably store a copy of the template before making the call into libuv

[targos]

I'd go with Richard's suggestion:

we should probably store a copy of the template before making the call into libuv

Okay, can you help me do it? I don't know how I could access the value from the AfterMkstemp callback.

[aduh95]

@targos does the change made to libuv by @vtjnash help unblock this?

[targos]

It doesn't. I still need help for #33549 (comment)

[mschfh]

it doesn't have to hold up this PR

If we deem it useful from a debugging/errors point of view in Node.js we should probably store a copy of the template before making the call into libuv.

Could this PR be merged and the enhancement be added later on?

[BrianJDrake]

This pull request would complete the old feature request #5332.

[targos]

Anyone feel free to take this over. I'm not able to fix the problems and I don't need the feature myself.