fs: pass ERR_DIR_CLOSED asynchronously to dir.close

Lxxyx · danielleadams · commit 744b8aa80736 · 2020-12-07T13:55:08.000-05:00
Pass the error to the callback or returns a rejected Promise instead of throwing a synchonous error. Fixes: #36237 PR-URL: #36243 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js
@@ -6,6 +6,7 @@ const {
   ArrayPrototypeSplice,
   FunctionPrototypeBind,
   ObjectDefineProperty,
+  PromiseReject,
   Symbol,
   SymbolAsyncIterator,
 } = primordials;
@@ -164,16 +165,24 @@ class Dir {
   }
 
   close(callback) {
-    if (this[kDirClosed] === true) {
-      throw new ERR_DIR_CLOSED();
-    }
-
+    // Promise
     if (callback === undefined) {
+      if (this[kDirClosed] === true) {
+        return PromiseReject(new ERR_DIR_CLOSED());
+      }
       return this[kDirClosePromisified]();
-    } else if (typeof callback !== 'function') {
+    }
+
+    // callback
+    if (typeof callback !== 'function') {
       throw new ERR_INVALID_CALLBACK(callback);
     }
 
+    if (this[kDirClosed] === true) {
+      process.nextTick(callback, new ERR_DIR_CLOSED());
+      return;
+    }
+
     if (this[kDirOperationQueue] !== null) {
       this[kDirOperationQueue].push(() => {
         this.close(callback);
diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js
@@ -223,12 +223,11 @@ async function doAsyncIterInvalidCallbackTest() {
 }
 doAsyncIterInvalidCallbackTest().then(common.mustCall());
 
-// Check if directory already closed - throw an exception
+// Check first call to close() - should not report an error.
 async function doAsyncIterDirClosedTest() {
   const dir = await fs.promises.opendir(testDir);
   await dir.close();
-
-  assert.throws(() => dir.close(), dirclosedError);
+  await assert.rejects(() => dir.close(), dirclosedError);
 }
 doAsyncIterDirClosedTest().then(common.mustCall());
 
@@ -267,3 +266,19 @@ async function doConcurrentAsyncMixedOps() {
   await promise2;
 }
 doConcurrentAsyncMixedOps().then(common.mustCall());
+
+// Check if directory already closed - the callback should pass an error.
+{
+  const dir = fs.opendirSync(testDir);
+  dir.closeSync();
+  dir.close(common.mustCall((error) => {
+    assert.strictEqual(error.code, dirclosedError.code);
+  }));
+}
+
+// Check if directory already closed - throw an promise exception.
+{
+  const dir = fs.opendirSync(testDir);
+  dir.closeSync();
+  assert.rejects(dir.close(), dirclosedError).then(common.mustCall());
+}
