Skip to content

Commit

Permalink
fs: return first folder made by mkdir recursive
Browse files Browse the repository at this point in the history
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
bcoe authored and targos committed Apr 28, 2020
1 parent e2a090b commit 7260ede
Show file tree
Hide file tree
Showing 7 changed files with 225 additions and 23 deletions.
14 changes: 9 additions & 5 deletions doc/api/fs.md
Expand Up @@ -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`
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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`
Expand Down
9 changes: 6 additions & 3 deletions lib/fs.js
Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions src/inspector_profiler.cc
Expand Up @@ -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));
Expand Down
6 changes: 6 additions & 0 deletions src/node_file-inl.h
Expand Up @@ -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());
Expand Down
105 changes: 93 additions & 12 deletions src/node_file.cc
Expand Up @@ -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<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);
Expand Down Expand Up @@ -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<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;
Expand All @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1362,6 +1409,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);

Expand All @@ -1380,14 +1446,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);
Expand Down
14 changes: 14 additions & 0 deletions src/node_file.h
Expand Up @@ -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)
Expand All @@ -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> {
Expand Down Expand Up @@ -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
Expand Down
94 changes: 94 additions & 0 deletions test/parallel/test-fs-mkdir.js
Expand Up @@ -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(() => {});

0 comments on commit 7260ede

Please sign in to comment.