Rodrigo Muino Tomonari requested to merge github/fork/anonrig/add-fast-close into main Jul 18, 2024

For those who are unfamiliar, a recent change of adding V8 Fast API to fs.closeSync broke almost all applications in v22.5.0. The goal of that PR was to significantly improve the performance of this function and speed up applications like NPM. Unfortunately, I broke it. For reference, here is the root cause of this incident: https://github.com/nodejs/node/pull/53627

Problem

Here's the fix for the bug breaking v22.5.0. Before diving into the technical implementation, let me explain what the bugs were.

There were actually 2 bugs causing this issue.

The crash

The first issue was the crash with the error message: ERROR: v8::Object::GetCreationContextChecked No creation context available. This error was caused by lib/internal/fs/read/context.js. We were destructuring the result of internalBinding('fs') which caused the creation context of the function being unavailable.

Previously, it was:

const { close } = internalBinding('fs')
close(fd, req)

The fix was:

const binding = internalBinding('fs')
binding.close(fd, req)

Breaking NPM

The second issue was caused by the addition of V8 Fast API call for fs.closeSync(), at least that's what I thought I was adding the fast API for. Unfortunately, adding a fast API to Close function, even though the fast API had a function signature of FastClose(recv, uin32_t fd, options), it was still getting triggered for fs.close(fd, req).

Unfortunately, this has been the expectation from my side, and unfortunately, under optimization, causing V8 Fast API to trigger it and causing NPM to fail.

The error message was:

npm error Exit handler never called!
npm error This is an error with npm itself. Please report this error at:

Since FastClose didn't receive any function as a parameter, we weren't executing it. This resulted in fs.close(fd, cb) to be executed in the fast API context, and causing NPM to fail, since it was expecting the result of the close operation.

Moving Sync and non-sync version to 2 different C++ functions and adding fast API to sync version fixes this problem.

CITGM

Node.js has a project called Canary in the gold mine (CITGM) to catch unintended bugs that might affect the ecosystem, like this. Unfortunately, recently we discovered that even if there were failures our CI jobs were successful. This issue is currently investigated under https://github.com/nodejs/citgm/issues/1067.

Solution

We need better testing for V8 Fast API implementations. There are some unintended consequences of these additions and unfortunately, it requires a more thorough pull-request review.

Thank you!

Admin message