From df501316c138387044262bb70f7225951185f324 Mon Sep 17 00:00:00 2001
From: Santiago Gimeno <santiago.gimeno@gmail.com>
Date: Sat, 18 Jun 2022 11:20:40 +0200
Subject: [PATCH] fs: don't end fs promises on Isolate termination

This is specially prevalent in the case of having in-progress FileHandle
operations in a worker thread while the Worker is exiting.

Fixes: https://github.com/nodejs/node/issues/42829

PR-URL: https://github.com/nodejs/node/pull/42910
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
---
 src/node_file-inl.h                           |  6 ++-
 src/node_file.cc                              |  5 ++
 ...t-worker-fshandles-error-on-termination.js | 51 +++++++++++++++++++
 ...ker-fshandles-open-close-on-termination.js | 46 +++++++++++++++++
 4 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-worker-fshandles-error-on-termination.js
 create mode 100644 test/parallel/test-worker-fshandles-open-close-on-termination.js

diff --git a/src/node_file-inl.h b/src/node_file-inl.h
index 0188261b7dcbb4..28d4d9ab8c8903 100644
--- a/src/node_file-inl.h
+++ b/src/node_file-inl.h
@@ -156,8 +156,10 @@ FSReqPromise<AliasedBufferT>::New(BindingData* binding_data,
 
 template <typename AliasedBufferT>
 FSReqPromise<AliasedBufferT>::~FSReqPromise() {
-  // Validate that the promise was explicitly resolved or rejected.
-  CHECK(finished_);
+  // Validate that the promise was explicitly resolved or rejected but only if
+  // the Isolate is not terminating because in this case the promise might have
+  // not finished.
+  if (!env()->is_stopping()) CHECK(finished_);
 }
 
 template <typename AliasedBufferT>
diff --git a/src/node_file.cc b/src/node_file.cc
index f4238e7d68cce0..5a3e54669049fb 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -377,6 +377,7 @@ MaybeLocal<Promise> FileHandle::ClosePromise() {
     std::unique_ptr<CloseReq> close(CloseReq::from_req(req));
     CHECK_NOT_NULL(close);
     close->file_handle()->AfterClose();
+    if (!close->env()->can_call_into_js()) return;
     Isolate* isolate = close->env()->isolate();
     if (req->result < 0) {
       HandleScope handle_scope(isolate);
@@ -650,6 +651,10 @@ void FSReqAfterScope::Reject(uv_fs_t* req) {
 }
 
 bool FSReqAfterScope::Proceed() {
+  if (!wrap_->env()->can_call_into_js()) {
+    return false;
+  }
+
   if (req_->result < 0) {
     Reject(req_);
     return false;
diff --git a/test/parallel/test-worker-fshandles-error-on-termination.js b/test/parallel/test-worker-fshandles-error-on-termination.js
new file mode 100644
index 00000000000000..72fb969963a2b0
--- /dev/null
+++ b/test/parallel/test-worker-fshandles-error-on-termination.js
@@ -0,0 +1,51 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs/promises');
+const { scheduler } = require('timers/promises');
+const { parentPort, Worker } = require('worker_threads');
+
+const MAX_ITERATIONS = 20;
+const MAX_THREADS = 10;
+
+// Do not use isMainThread so that this test itself can be run inside a Worker.
+if (!process.env.HAS_STARTED_WORKER) {
+  process.env.HAS_STARTED_WORKER = 1;
+
+  function spinWorker(iter) {
+    const w = new Worker(__filename);
+    w.on('message', common.mustCall((msg) => {
+      assert.strictEqual(msg, 'terminate');
+      w.terminate();
+    }));
+
+    w.on('exit', common.mustCall(() => {
+      if (iter < MAX_ITERATIONS)
+        spinWorker(++iter);
+    }));
+  }
+
+  for (let i = 0; i < MAX_THREADS; i++) {
+    spinWorker(0);
+  }
+} else {
+  async function open_nok() {
+    await assert.rejects(
+      fs.open('this file does not exist'),
+      {
+        code: 'ENOENT',
+        syscall: 'open'
+      }
+    );
+    await scheduler.yield();
+    await open_nok();
+  }
+
+  // These async function calls never return as they are meant to continually
+  // open nonexistent files until the worker is terminated.
+  open_nok();
+  open_nok();
+
+  parentPort.postMessage('terminate');
+}
diff --git a/test/parallel/test-worker-fshandles-open-close-on-termination.js b/test/parallel/test-worker-fshandles-open-close-on-termination.js
new file mode 100644
index 00000000000000..2cbd886b15fe4e
--- /dev/null
+++ b/test/parallel/test-worker-fshandles-open-close-on-termination.js
@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs/promises');
+const { scheduler } = require('timers/promises');
+const { parentPort, Worker } = require('worker_threads');
+
+const MAX_ITERATIONS = 20;
+const MAX_THREADS = 10;
+
+// Do not use isMainThread so that this test itself can be run inside a Worker.
+if (!process.env.HAS_STARTED_WORKER) {
+  process.env.HAS_STARTED_WORKER = 1;
+
+  function spinWorker(iter) {
+    const w = new Worker(__filename);
+    w.on('message', common.mustCall((msg) => {
+      assert.strictEqual(msg, 'terminate');
+      w.terminate();
+    }));
+
+    w.on('exit', common.mustCall(() => {
+      if (iter < MAX_ITERATIONS)
+        spinWorker(++iter);
+    }));
+  }
+
+  for (let i = 0; i < MAX_THREADS; i++) {
+    spinWorker(0);
+  }
+} else {
+  async function open_close() {
+    const fh = await fs.open(__filename);
+    await fh.close();
+    await scheduler.yield();
+    await open_close();
+  }
+
+  // These async function calls never return as they are meant to continually
+  // open and close files until the worker is terminated.
+  open_close();
+  open_close();
+
+  parentPort.postMessage('terminate');
+}