From 7815bef2c946672815a8c3f2d8075eb61602e421 Mon Sep 17 00:00:00 2001 From: bcoe Date: Tue, 28 Jan 2020 23:06:44 -0800 Subject: [PATCH] fs: return first folder made by mkdir recursive mkdir('/foo/bar', { recursive: true }) and mkdirSync will now return the path of the first folder created. This matches more closely mkdirp's behavior, and is useful for setting folder permissions. PR-URL: https://github.com/nodejs/node/pull/31530 Refs: https://github.com/nodejs/node/issues/31481 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott --- doc/api/fs.md | 14 +++-- lib/fs.js | 9 ++- src/inspector_profiler.cc | 6 +- src/node_file-inl.h | 6 ++ src/node_file.cc | 105 +++++++++++++++++++++++++++++---- src/node_file.h | 14 +++++ test/parallel/test-fs-mkdir.js | 94 +++++++++++++++++++++++++++++ 7 files changed, 225 insertions(+), 23 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index f2221c9eae45be..5070d99611889e 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2463,8 +2463,10 @@ changes: * `callback` {Function} * `err` {Error} -Asynchronously creates a directory. No arguments other than a possible exception -are given to the completion callback. +Asynchronously creates a directory. + +The callback is given a possible exception and, if `recursive` is `true`, the +first folder path created, `(err, [path])`. The optional `options` argument can be an integer specifying mode (permission and sticky bits), or an object with a `mode` property and a `recursive` @@ -2508,8 +2510,10 @@ changes: * `options` {Object|integer} * `recursive` {boolean} **Default:** `false` * `mode` {string|integer} Not supported on Windows. **Default:** `0o777`. +* Returns: {string|undefined} -Synchronously creates a directory. Returns `undefined`. +Synchronously creates a directory. Returns `undefined`, or if `recursive` is +`true`, the first folder path created. This is the synchronous version of [`fs.mkdir()`][]. See also: mkdir(2). @@ -4818,8 +4822,8 @@ added: v10.0.0 * `mode` {string|integer} Not supported on Windows. **Default:** `0o777`. * Returns: {Promise} -Asynchronously creates a directory then resolves the `Promise` with no -arguments upon success. +Asynchronously creates a directory then resolves the `Promise` with either no +arguments, or the first folder path created if `recursive` is `true`. The optional `options` argument can be an integer specifying mode (permission and sticky bits), or an object with a `mode` property and a `recursive` diff --git a/lib/fs.js b/lib/fs.js index 5fb1784605700e..77b629e4757477 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -835,10 +835,13 @@ function mkdirSync(path, options) { throw new ERR_INVALID_ARG_TYPE('options.recursive', 'boolean', recursive); const ctx = { path }; - binding.mkdir(pathModule.toNamespacedPath(path), - parseMode(mode, 'mode', 0o777), recursive, undefined, - ctx); + const result = binding.mkdir(pathModule.toNamespacedPath(path), + parseMode(mode, 'mode', 0o777), recursive, + undefined, ctx); handleErrorFromBinding(ctx); + if (recursive) { + return result; + } } function readdir(path, options, callback) { diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index f9d3c2b512f1c1..cc4c3091757683 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -107,9 +107,9 @@ void V8ProfilerConnection::V8ProfilerSessionDelegate::SendMessageToFrontend( } static bool EnsureDirectory(const std::string& directory, const char* type) { - uv_fs_t req; - int ret = fs::MKDirpSync(nullptr, &req, directory, 0777, nullptr); - uv_fs_req_cleanup(&req); + fs::FSReqWrapSync req_wrap_sync; + int ret = fs::MKDirpSync(nullptr, &req_wrap_sync.req, directory, 0777, + nullptr); if (ret < 0 && ret != UV_EEXIST) { char err_buf[128]; uv_err_name_r(ret, err_buf, sizeof(err_buf)); diff --git a/src/node_file-inl.h b/src/node_file-inl.h index 390f6c7415770f..e9ed18a75fa48b 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -21,6 +21,12 @@ void FSContinuationData::PushPath(const std::string& path) { paths_.push_back(path); } +void FSContinuationData::MaybeSetFirstPath(const std::string& path) { + if (first_path_.empty()) { + first_path_ = path; + } +} + std::string FSContinuationData::PopPath() { CHECK_GT(paths_.size(), 0); std::string path = std::move(paths_.back()); diff --git a/src/node_file.cc b/src/node_file.cc index bfdc73c9371ad1..a109aea7092148 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -590,6 +590,43 @@ void AfterOpenFileHandle(uv_fs_t* req) { } } +// Reverse the logic applied by path.toNamespacedPath() to create a +// namespace-prefixed path. +void FromNamespacedPath(std::string* path) { +#ifdef _WIN32 + if (path->compare(0, 8, "\\\\?\\UNC\\", 8) == 0) { + *path = path->substr(8); + path->insert(0, "\\\\"); + } else if (path->compare(0, 4, "\\\\?\\", 4) == 0) { + *path = path->substr(4); + } +#endif +} + +void AfterMkdirp(uv_fs_t* req) { + FSReqBase* req_wrap = FSReqBase::from_req(req); + FSReqAfterScope after(req_wrap, req); + + MaybeLocal path; + Local error; + + if (after.Proceed()) { + if (!req_wrap->continuation_data()->first_path().empty()) { + std::string first_path(req_wrap->continuation_data()->first_path()); + FromNamespacedPath(&first_path); + path = StringBytes::Encode(req_wrap->env()->isolate(), first_path.c_str(), + req_wrap->encoding(), + &error); + if (path.IsEmpty()) + req_wrap->Reject(error); + else + req_wrap->Resolve(path.ToLocalChecked()); + } else { + req_wrap->Resolve(Undefined(req_wrap->env()->isolate())); + } + } +} + void AfterStringPath(uv_fs_t* req) { FSReqBase* req_wrap = FSReqBase::from_req(req); FSReqAfterScope after(req_wrap, req); @@ -1215,18 +1252,25 @@ int MKDirpSync(uv_loop_t* loop, const std::string& path, int mode, uv_fs_cb cb) { - FSContinuationData continuation_data(req, mode, cb); - continuation_data.PushPath(std::move(path)); + FSReqWrapSync* req_wrap = ContainerOf(&FSReqWrapSync::req, req); + + // on the first iteration of algorithm, stash state information. + if (req_wrap->continuation_data() == nullptr) { + req_wrap->set_continuation_data( + std::make_unique(req, mode, cb)); + req_wrap->continuation_data()->PushPath(std::move(path)); + } - while (continuation_data.paths().size() > 0) { - std::string next_path = continuation_data.PopPath(); + while (req_wrap->continuation_data()->paths().size() > 0) { + std::string next_path = req_wrap->continuation_data()->PopPath(); int err = uv_fs_mkdir(loop, req, next_path.c_str(), mode, nullptr); while (true) { switch (err) { // Note: uv_fs_req_cleanup in terminal paths will be called by // ~FSReqWrapSync(): case 0: - if (continuation_data.paths().size() == 0) { + req_wrap->continuation_data()->MaybeSetFirstPath(next_path); + if (req_wrap->continuation_data()->paths().size() == 0) { return 0; } break; @@ -1239,9 +1283,9 @@ int MKDirpSync(uv_loop_t* loop, std::string dirname = next_path.substr(0, next_path.find_last_of(kPathSeparator)); if (dirname != next_path) { - continuation_data.PushPath(std::move(next_path)); - continuation_data.PushPath(std::move(dirname)); - } else if (continuation_data.paths().size() == 0) { + req_wrap->continuation_data()->PushPath(std::move(next_path)); + req_wrap->continuation_data()->PushPath(std::move(dirname)); + } else if (req_wrap->continuation_data()->paths().size() == 0) { err = UV_EEXIST; continue; } @@ -1253,7 +1297,8 @@ int MKDirpSync(uv_loop_t* loop, err = uv_fs_stat(loop, req, next_path.c_str(), nullptr); if (err == 0 && !S_ISDIR(req->statbuf.st_mode)) { uv_fs_req_cleanup(req); - if (orig_err == UV_EEXIST && continuation_data.paths().size() > 0) { + if (orig_err == UV_EEXIST && + req_wrap->continuation_data()->paths().size() > 0) { return UV_ENOTDIR; } return UV_EEXIST; @@ -1298,8 +1343,10 @@ int MKDirpAsync(uv_loop_t* loop, // FSReqAfterScope::~FSReqAfterScope() case 0: { if (req_wrap->continuation_data()->paths().size() == 0) { + req_wrap->continuation_data()->MaybeSetFirstPath(path); req_wrap->continuation_data()->Done(0); } else { + req_wrap->continuation_data()->MaybeSetFirstPath(path); uv_fs_req_cleanup(req); MKDirpAsync(loop, req, path.c_str(), req_wrap->continuation_data()->mode(), nullptr); @@ -1362,6 +1409,25 @@ int MKDirpAsync(uv_loop_t* loop, return err; } +int CallMKDirpSync(Environment* env, const FunctionCallbackInfo& args, + FSReqWrapSync* req_wrap, const char* path, int mode) { + env->PrintSyncTrace(); + int err = MKDirpSync(env->event_loop(), &req_wrap->req, path, mode, + nullptr); + if (err < 0) { + v8::Local context = env->context(); + v8::Local ctx_obj = args[4].As(); + v8::Isolate* isolate = env->isolate(); + ctx_obj->Set(context, + env->errno_string(), + v8::Integer::New(isolate, err)).Check(); + ctx_obj->Set(context, + env->syscall_string(), + OneByteString(isolate, "mkdir")).Check(); + } + return err; +} + static void MKDir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1380,14 +1446,29 @@ static void MKDir(const FunctionCallbackInfo& args) { FSReqBase* req_wrap_async = GetReqWrap(env, args[3]); if (req_wrap_async != nullptr) { // mkdir(path, mode, req) AsyncCall(env, req_wrap_async, args, "mkdir", UTF8, - AfterNoArgs, mkdirp ? MKDirpAsync : uv_fs_mkdir, *path, mode); + mkdirp ? AfterMkdirp : AfterNoArgs, + mkdirp ? MKDirpAsync : uv_fs_mkdir, *path, mode); } else { // mkdir(path, mode, undefined, ctx) CHECK_EQ(argc, 5); FSReqWrapSync req_wrap_sync; FS_SYNC_TRACE_BEGIN(mkdir); if (mkdirp) { - SyncCall(env, args[4], &req_wrap_sync, "mkdir", - MKDirpSync, *path, mode); + int err = CallMKDirpSync(env, args, &req_wrap_sync, *path, mode); + if (err == 0 && + !req_wrap_sync.continuation_data()->first_path().empty()) { + Local error; + std::string first_path(req_wrap_sync.continuation_data()->first_path()); + FromNamespacedPath(&first_path); + MaybeLocal path = StringBytes::Encode(env->isolate(), + first_path.c_str(), + UTF8, &error); + if (path.IsEmpty()) { + Local ctx = args[4].As(); + ctx->Set(env->context(), env->error_string(), error).Check(); + return; + } + args.GetReturnValue().Set(path.ToLocalChecked()); + } } else { SyncCall(env, args[4], &req_wrap_sync, "mkdir", uv_fs_mkdir, *path, mode); diff --git a/src/node_file.h b/src/node_file.h index 0574aa8f6c079d..1ebfe06c1fbdbf 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -19,10 +19,13 @@ class FSContinuationData : public MemoryRetainer { inline void PushPath(std::string&& path); inline void PushPath(const std::string& path); inline std::string PopPath(); + // Used by mkdirp to track the first path created: + inline void MaybeSetFirstPath(const std::string& path); inline void Done(int result); int mode() const { return mode_; } const std::vector& paths() const { return paths_; } + const std::string& first_path() const { return first_path_; } void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(FSContinuationData) @@ -33,6 +36,7 @@ class FSContinuationData : public MemoryRetainer { uv_fs_t* req_; int mode_; std::vector paths_; + std::string first_path_; }; class FSReqBase : public ReqWrap { @@ -306,8 +310,18 @@ class FSReqWrapSync { ~FSReqWrapSync() { uv_fs_req_cleanup(&req); } uv_fs_t req; + FSContinuationData* continuation_data() const { + return continuation_data_.get(); + } + void set_continuation_data(std::unique_ptr data) { + continuation_data_ = std::move(data); + } + FSReqWrapSync(const FSReqWrapSync&) = delete; FSReqWrapSync& operator=(const FSReqWrapSync&) = delete; + + private: + std::unique_ptr continuation_data_; }; // TODO(addaleax): Currently, callers check the return value and assume diff --git a/test/parallel/test-fs-mkdir.js b/test/parallel/test-fs-mkdir.js index 417f5c7e06b8b5..5e28d6b944b6e0 100644 --- a/test/parallel/test-fs-mkdir.js +++ b/test/parallel/test-fs-mkdir.js @@ -245,6 +245,100 @@ if (common.isMainThread && (common.isLinux || common.isOSX)) { }); } +// `mkdirp` returns first folder created, when all folders are new. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const firstPathCreated = path.join(tmpdir.path, dir1); + const pathname = path.join(tmpdir.path, dir1, dir2); + + fs.mkdir(pathname, { recursive: true }, common.mustCall(function(err, path) { + assert.strictEqual(err, null); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(path, firstPathCreated); + })); +} + +// `mkdirp` returns first folder created, when last folder is new. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const pathname = path.join(tmpdir.path, dir1, dir2); + fs.mkdirSync(path.join(tmpdir.path, dir1)); + fs.mkdir(pathname, { recursive: true }, common.mustCall(function(err, path) { + assert.strictEqual(err, null); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(path, pathname); + })); +} + +// `mkdirp` returns undefined, when no new folders are created. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const pathname = path.join(tmpdir.path, dir1, dir2); + fs.mkdirSync(path.join(tmpdir.path, dir1, dir2), { recursive: true }); + fs.mkdir(pathname, { recursive: true }, common.mustCall(function(err, path) { + assert.strictEqual(err, null); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(path, undefined); + })); +} + +// `mkdirp.sync` returns first folder created, when all folders are new. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const firstPathCreated = path.join(tmpdir.path, dir1); + const pathname = path.join(tmpdir.path, dir1, dir2); + const p = fs.mkdirSync(pathname, { recursive: true }); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(p, firstPathCreated); +} + +// `mkdirp.sync` returns first folder created, when last folder is new. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const pathname = path.join(tmpdir.path, dir1, dir2); + fs.mkdirSync(path.join(tmpdir.path, dir1), { recursive: true }); + const p = fs.mkdirSync(pathname, { recursive: true }); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(p, pathname); +} + +// `mkdirp.sync` returns undefined, when no new folders are created. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const pathname = path.join(tmpdir.path, dir1, dir2); + fs.mkdirSync(path.join(tmpdir.path, dir1, dir2), { recursive: true }); + const p = fs.mkdirSync(pathname, { recursive: true }); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(p, undefined); +} + +// `mkdirp.promises` returns first folder created, when all folders are new. +{ + const dir1 = nextdir(); + const dir2 = nextdir(); + const firstPathCreated = path.join(tmpdir.path, dir1); + const pathname = path.join(tmpdir.path, dir1, dir2); + async function testCase() { + const p = await fs.promises.mkdir(pathname, { recursive: true }); + assert.strictEqual(fs.existsSync(pathname), true); + assert.strictEqual(fs.statSync(pathname).isDirectory(), true); + assert.strictEqual(p, firstPathCreated); + } + testCase(); +} + // Keep the event loop alive so the async mkdir() requests // have a chance to run (since they don't ref the event loop). process.nextTick(() => {});