diff --git a/doc/api/errors.md b/doc/api/errors.md
index 172eadf7686794..ecd76a653e029c 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -855,6 +855,15 @@ An unknown Diffie-Hellman group name was given. See
The [`fs.Dir`][] was previously closed.
+
+### `ERR_DIR_CONCURRENT_OPERATION`
+
+
+A synchronous read or close call was attempted on an [`fs.Dir`][] which has
+ongoing asynchronous operations.
+
### `ERR_DNS_SET_SERVERS_FAILED`
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 2a162f14b24206..ae78b6a517a918 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -788,6 +788,9 @@ E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error);
E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
'Input buffers must have the same byte length', RangeError);
E('ERR_DIR_CLOSED', 'Directory handle was closed', Error);
+E('ERR_DIR_CONCURRENT_OPERATION',
+ 'Cannot do synchronous work on directory handle with concurrent ' +
+ 'asynchronous operations', Error);
E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]',
Error);
E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js
index e23b2f7725b2a2..b4cd72d3668e25 100644
--- a/lib/internal/fs/dir.js
+++ b/lib/internal/fs/dir.js
@@ -12,6 +12,7 @@ const dirBinding = internalBinding('fs_dir');
const {
codes: {
ERR_DIR_CLOSED,
+ ERR_DIR_CONCURRENT_OPERATION,
ERR_INVALID_CALLBACK,
ERR_MISSING_ARGS
}
@@ -37,6 +38,7 @@ const kDirOptions = Symbol('kDirOptions');
const kDirReadImpl = Symbol('kDirReadImpl');
const kDirReadPromisified = Symbol('kDirReadPromisified');
const kDirClosePromisified = Symbol('kDirClosePromisified');
+const kDirOperationQueue = Symbol('kDirOperationQueue');
class Dir {
constructor(handle, path, options) {
@@ -46,6 +48,10 @@ class Dir {
this[kDirPath] = path;
this[kDirClosed] = false;
+ // Either `null` or an Array of pending operations (= functions to be called
+ // once the current operation is done).
+ this[kDirOperationQueue] = null;
+
this[kDirOptions] = {
bufferSize: 32,
...getOptions(options, {
@@ -79,6 +85,13 @@ class Dir {
throw new ERR_INVALID_CALLBACK(callback);
}
+ if (this[kDirOperationQueue] !== null) {
+ this[kDirOperationQueue].push(() => {
+ this[kDirReadImpl](maybeSync, callback);
+ });
+ return;
+ }
+
if (this[kDirBufferedEntries].length > 0) {
const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
if (maybeSync)
@@ -90,6 +103,12 @@ class Dir {
const req = new FSReqCallback();
req.oncomplete = (err, result) => {
+ process.nextTick(() => {
+ const queue = this[kDirOperationQueue];
+ this[kDirOperationQueue] = null;
+ for (const op of queue) op();
+ });
+
if (err || result === null) {
return callback(err, result);
}
@@ -98,6 +117,7 @@ class Dir {
getDirent(this[kDirPath], result[0], result[1], callback);
};
+ this[kDirOperationQueue] = [];
this[kDirHandle].read(
this[kDirOptions].encoding,
this[kDirOptions].bufferSize,
@@ -110,6 +130,10 @@ class Dir {
throw new ERR_DIR_CLOSED();
}
+ if (this[kDirOperationQueue] !== null) {
+ throw new ERR_DIR_CONCURRENT_OPERATION();
+ }
+
if (this[kDirBufferedEntries].length > 0) {
const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
return getDirent(this[kDirPath], name, type);
@@ -143,6 +167,13 @@ class Dir {
throw new ERR_INVALID_CALLBACK(callback);
}
+ if (this[kDirOperationQueue] !== null) {
+ this[kDirOperationQueue].push(() => {
+ this.close(callback);
+ });
+ return;
+ }
+
this[kDirClosed] = true;
const req = new FSReqCallback();
req.oncomplete = callback;
@@ -154,6 +185,10 @@ class Dir {
throw new ERR_DIR_CLOSED();
}
+ if (this[kDirOperationQueue] !== null) {
+ throw new ERR_DIR_CONCURRENT_OPERATION();
+ }
+
this[kDirClosed] = true;
const ctx = { path: this[kDirPath] };
const result = this[kDirHandle].close(undefined, ctx);
diff --git a/src/node_dir.cc b/src/node_dir.cc
index c4aaf4bcd3e8ba..b0904c3d39ddd0 100644
--- a/src/node_dir.cc
+++ b/src/node_dir.cc
@@ -197,8 +197,8 @@ static MaybeLocal DirentListToArray(
}
static void AfterDirRead(uv_fs_t* req) {
- FSReqBase* req_wrap = FSReqBase::from_req(req);
- FSReqAfterScope after(req_wrap, req);
+ BaseObjectPtr req_wrap { FSReqBase::from_req(req) };
+ FSReqAfterScope after(req_wrap.get(), req);
if (!after.Proceed()) {
return;
@@ -210,12 +210,12 @@ static void AfterDirRead(uv_fs_t* req) {
if (req->result == 0) {
// Done
Local done = Null(isolate);
+ after.Clear();
req_wrap->Resolve(done);
return;
}
uv_dir_t* dir = static_cast(req->ptr);
- req->ptr = nullptr;
Local error;
Local js_array;
@@ -224,9 +224,13 @@ static void AfterDirRead(uv_fs_t* req) {
req->result,
req_wrap->encoding(),
&error).ToLocal(&js_array)) {
+ // Clear libuv resources *before* delivering results to JS land because
+ // that can schedule another operation on the same uv_dir_t. Ditto below.
+ after.Clear();
return req_wrap->Reject(error);
}
+ after.Clear();
req_wrap->Resolve(js_array);
}
diff --git a/src/node_file.cc b/src/node_file.cc
index be18b18fddd413..96adbc9e756688 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -556,8 +556,15 @@ FSReqAfterScope::FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req)
}
FSReqAfterScope::~FSReqAfterScope() {
+ Clear();
+}
+
+void FSReqAfterScope::Clear() {
+ if (!wrap_) return;
+
uv_fs_req_cleanup(wrap_->req());
- delete wrap_;
+ wrap_->Detach();
+ wrap_.reset();
}
// TODO(joyeecheung): create a normal context object, and
@@ -570,12 +577,16 @@ FSReqAfterScope::~FSReqAfterScope() {
// which is also why the errors should have been constructed
// in JS for more flexibility.
void FSReqAfterScope::Reject(uv_fs_t* req) {
- wrap_->Reject(UVException(wrap_->env()->isolate(),
- req->result,
- wrap_->syscall(),
- nullptr,
- req->path,
- wrap_->data()));
+ BaseObjectPtr wrap { wrap_ };
+ Local exception =
+ UVException(wrap_->env()->isolate(),
+ req->result,
+ wrap_->syscall(),
+ nullptr,
+ req->path,
+ wrap_->data());
+ Clear();
+ wrap->Reject(exception);
}
bool FSReqAfterScope::Proceed() {
diff --git a/src/node_file.h b/src/node_file.h
index 7690f272848425..39c9034f669ea7 100644
--- a/src/node_file.h
+++ b/src/node_file.h
@@ -184,6 +184,7 @@ class FSReqAfterScope final {
public:
FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req);
~FSReqAfterScope();
+ void Clear();
bool Proceed();
@@ -195,7 +196,7 @@ class FSReqAfterScope final {
FSReqAfterScope& operator=(const FSReqAfterScope&&) = delete;
private:
- FSReqBase* wrap_ = nullptr;
+ BaseObjectPtr wrap_;
uv_fs_t* req_ = nullptr;
v8::HandleScope handle_scope_;
v8::Context::Scope context_scope_;
diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js
index 733d9c7f3e0487..e5233e3a10914a 100644
--- a/test/parallel/test-fs-opendir.js
+++ b/test/parallel/test-fs-opendir.js
@@ -33,6 +33,10 @@ const dirclosedError = {
code: 'ERR_DIR_CLOSED'
};
+const dirconcurrentError = {
+ code: 'ERR_DIR_CONCURRENT_OPERATION'
+};
+
const invalidCallbackObj = {
code: 'ERR_INVALID_CALLBACK',
name: 'TypeError'
@@ -230,3 +234,39 @@ async function doAsyncIterDirClosedTest() {
assert.throws(() => dir.close(), dirclosedError);
}
doAsyncIterDirClosedTest().then(common.mustCall());
+
+// Check that readSync() and closeSync() during read() throw exceptions
+async function doConcurrentAsyncAndSyncOps() {
+ const dir = await fs.promises.opendir(testDir);
+ const promise = dir.read();
+
+ assert.throws(() => dir.closeSync(), dirconcurrentError);
+ assert.throws(() => dir.readSync(), dirconcurrentError);
+
+ await promise;
+ dir.closeSync();
+}
+doConcurrentAsyncAndSyncOps().then(common.mustCall());
+
+// Check that concurrent read() operations don't do weird things.
+async function doConcurrentAsyncOps() {
+ const dir = await fs.promises.opendir(testDir);
+ const promise1 = dir.read();
+ const promise2 = dir.read();
+
+ assertDirent(await promise1);
+ assertDirent(await promise2);
+ dir.closeSync();
+}
+doConcurrentAsyncOps().then(common.mustCall());
+
+// Check that concurrent read() + close() operations don't do weird things.
+async function doConcurrentAsyncMixedOps() {
+ const dir = await fs.promises.opendir(testDir);
+ const promise1 = dir.read();
+ const promise2 = dir.close();
+
+ assertDirent(await promise1);
+ await promise2;
+}
+doConcurrentAsyncMixedOps().then(common.mustCall());