fs: fix writeFile signal does not close file

Fix an issue where the writeFile does not close the file when the signal is aborted
nodejs · Feb 18, 2021 · a7f00e3 · a7f00e3
1 parent 88d9268
commit a7f00e3
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 6 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -338,6 +338,11 @@ function readFile(path, options, callback) {
     return;
   }
 
+  if (options.signal?.aborted) {
+    callback(lazyDOMException('The operation was aborted', 'AbortError'));
+    return;
+  }
+
   const flagsNumber = stringToFlags(options.flag);
   path = getValidatedPath(path);
 
@@ -1448,7 +1453,13 @@ function lutimesSync(path, atime, mtime) {
 
 function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
   if (signal?.aborted) {
-    callback(lazyDOMException('The operation was aborted', 'AbortError'));
+    if (isUserFd) {
+      callback(lazyDOMException('The operation was aborted', 'AbortError'));
+    } else {
+      fs.close(fd, function() {
+        callback(lazyDOMException('The operation was aborted', 'AbortError'));
+      });
+    }
     return;
   }
   // write(fd, buffer, offset, length, position, callback)

diff --git a/test/parallel/test-fs-write-file.js b/test/parallel/test-fs-write-file.js
@@ -24,8 +24,10 @@ const common = require('../common');
 const assert = require('assert');
 const fs = require('fs');
 const join = require('path').join;
-
+const cp = require('child_process');
+const os = require('os');
 const tmpdir = require('../common/tmpdir');
+
 tmpdir.refresh();
 
 const filename = join(tmpdir.path, 'test.txt');
@@ -67,31 +69,60 @@ fs.open(filename4, 'w+', common.mustSucceed((fd) => {
   }));
 }));
 
+function checkIfFileIsOpen(fileName) {
+  const platform = os.platform();
+  if (platform === 'linux' || platform === 'darwin') {
+    // Tried other ways like rm/stat, but that didn't work.
+    cp.exec(`lsof -p ${process.pid}`, common.mustCall((err, value) => {
+      if (err) {
+        assert.ifError(err);
+        return;
+      }
+      assert.ok(!value.toString().includes(fileName),
+                `${fileName} is still open, but should be closed`);
+    }));
+  }
+}
 
 {
   // Test that writeFile is cancellable with an AbortSignal.
-  // Before the operation has started
+  // abort sync after the operation has started
   const controller = new AbortController();
   const signal = controller.signal;
   const filename3 = join(tmpdir.path, 'test3.txt');
 
   fs.writeFile(filename3, s, { signal }, common.mustCall((err) => {
     assert.strictEqual(err.name, 'AbortError');
+    checkIfFileIsOpen(filename3);
   }));
 
   controller.abort();
 }
 
 {
   // Test that writeFile is cancellable with an AbortSignal.
-  // After the operation has started
+  // abort async after the operation has started
   const controller = new AbortController();
   const signal = controller.signal;
-  const filename4 = join(tmpdir.path, 'test5.txt');
+  const filename5 = join(tmpdir.path, 'test5.txt');
 
-  fs.writeFile(filename4, s, { signal }, common.mustCall((err) => {
+  fs.writeFile(filename5, s, { signal }, common.mustCall((err) => {
     assert.strictEqual(err.name, 'AbortError');
+    checkIfFileIsOpen(filename5);
   }));
 
   process.nextTick(() => controller.abort());
 }
+
+{
+  // Test that writeFile is cancellable with an AbortSignal.
+  // abort before the operation has started
+  const controller = new AbortController();
+  const signal = controller.signal;
+  const filename6 = join(tmpdir.path, 'test6.txt');
+  controller.abort();
+  fs.writeFile(filename6, s, { signal }, common.mustCall((err) => {
+    assert.strictEqual(err.name, 'AbortError');
+    checkIfFileIsOpen(filename6);
+  }));
+}