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: #31530 Refs: #31481 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Mar 10, 2020 · ff58854 · ff58854
1 parent 607ac90
commit ff58854
Show file tree

Hide file tree

Showing 7 changed files with 225 additions and 23 deletions.
diff --git 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).
@@ -4815,8 +4819,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
@@ -855,10 +855,13 @@ function mkdirSync(path, options) {
     throw new ERR_INVALID_ARG_TYPE('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
@@ -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
@@ -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
@@ -586,6 +586,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<Value> path;
+  Local<Value> 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);
@@ -1213,18 +1250,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<FSContinuationData>(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;
@@ -1237,9 +1281,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;
           }
@@ -1251,7 +1295,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;
@@ -1296,8 +1341,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);
@@ -1360,6 +1407,25 @@ int MKDirpAsync(uv_loop_t* loop,
   return err;
 }
 
+int CallMKDirpSync(Environment* env, const FunctionCallbackInfo<Value>& 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<v8::Context> context = env->context();
+    v8::Local<v8::Object> ctx_obj = args[4].As<v8::Object>();
+    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<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -1378,14 +1444,29 @@ static void MKDir(const FunctionCallbackInfo<Value>& 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<Value> error;
+        std::string first_path(req_wrap_sync.continuation_data()->first_path());
+        FromNamespacedPath(&first_path);
+        MaybeLocal<Value> path = StringBytes::Encode(env->isolate(),
+                                                     first_path.c_str(),
+                                                     UTF8, &error);
+        if (path.IsEmpty()) {
+          Local<Object> ctx = args[4].As<Object>();
+          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
@@ -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<std::string>& 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<std::string> paths_;
+  std::string first_path_;
 };
 
 class FSReqBase : public ReqWrap<uv_fs_t> {
@@ -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<FSContinuationData> data) {
+    continuation_data_ = std::move(data);
+  }
+
   FSReqWrapSync(const FSReqWrapSync&) = delete;
   FSReqWrapSync& operator=(const FSReqWrapSync&) = delete;
+
+ private:
+  std::unique_ptr<FSContinuationData> 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
@@ -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(() => {});