From 10ebd2a251363af1c6e1acd222ccb90868256584 Mon Sep 17 00:00:00 2001 From: Christian Aurich Zanettini Martins Date: Thu, 22 Oct 2026 19:15:53 -0300 Subject: [PATCH] fs: keep readFile abortable on non-regular files `ReadFileJob` treats a file that is not a regular file as size 0 and reads it to EOF inside one thread pool job, so the `AbortSignal` is only checked once the job ends. `fs.readFile()` and `fs.promises.readFile()` on a FIFO whose writer stays open therefore do not settle until the writer closes, and on `/dev/zero` they keep growing the buffer until the process runs out of memory. Hand the fd back to the chunked reader for non-regular files, the path files larger than one chunk already take. It checks the signal before each read. Regular files that report size 0, such as procfs files, are still read in one job. Signed-off-by: Christian Aurich Zanettini Martins --- lib/fs.js | 12 +++-- lib/internal/fs/promises.js | 6 +-- src/node_file.cc | 20 ++++---- test/parallel/test-fs-readfile-fifo-abort.js | 48 ++++++++++++++++++++ 4 files changed, 69 insertions(+), 17 deletions(-) create mode 100644 test/parallel/test-fs-readfile-fifo-abort.js diff --git a/lib/fs.js b/lib/fs.js index 9f6cfbddb657..51c574cb3ede 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -451,10 +451,11 @@ function readFile(path, options, callback) { const flagsNumber = stringToFlags(options.flag, 'options.flag'); path = getValidatedPath(path); if (options.buffer === undefined) { - // Open + fstat + read + close in one thread pool round trip for files of - // up to one chunk; larger files come back as an open fd + size and take - // the chunked reader below (readFileAfterOneShot). `true`: a handed-back - // fd will be closed through fs.close(), so track it as unmanaged. + // Open + fstat + read + close in one thread pool round trip for regular + // files of up to one chunk; larger and non-regular files (size 0) come + // back as an open fd + size and take the chunked reader below + // (readFileAfterOneShot). `true`: a handed-back fd will be closed through + // fs.close(), so track it as unmanaged. const job = new ReadFileJob(path, flagsNumber, kReadFileBufferLength, true); job.context = context; job.ondone = readFileAfterOneShot; @@ -479,7 +480,8 @@ function readFileAfterOneShot(err, buffer, fd, size, closeErr) { } if (fd !== -1) { // (context.read() below performs the abort check for this case.) - // Larger than one chunk: continue exactly like after open + fstat. + // Larger than one chunk or not a regular file: continue exactly like + // after open + fstat. context.fd = fd; context.size = size; if (size > kIoMaxLength) { diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 47574a5a4711..35268b33496f 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -2305,9 +2305,9 @@ async function readFile(path, options) { checkAborted(options.signal); if (options.buffer === undefined && vfsState.handlers === null) { - // Open + fstat + read + close in one thread pool round trip for files of - // up to one chunk; larger files come back as an open fd + size and are - // read by readFileHandle() as before. + // Open + fstat + read + close in one thread pool round trip for regular + // files of up to one chunk; larger and non-regular files (size 0) come + // back as an open fd + size and are read by readFileHandle() as before. path = getValidatedPath(path); const { 0: buffer, 1: fd, 2: size } = await readFileInOneRoundTrip(path, stringToFlags(flag)); if (fd === -1) { diff --git a/src/node_file.cc b/src/node_file.cc index f5e7cc80e724..85327608bd28 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3613,18 +3613,19 @@ static void ReadFileUtf8(const FunctionCallbackInfo& args) { // read + close -- instead of one round trip per step, which is what dominates // fs.readFile() for the typical small file and multiplies thread pool // contention when many files are read at once. Files whose size exceeds -// `limit` are not read here: the job hands the open fd and the size back so -// that the caller continues with the chunked reader (which stays fair and -// abortable for large files); it then only saved the fstat() round trip. +// `limit`, and files that are not regular files (a FIFO or a device may never +// reach EOF), are not read here: the job hands the open fd and the size back +// so that the caller continues with the chunked reader (which stays fair and +// abortable between chunks); it then only saved the fstat() round trip. // // JS: const job = new ReadFileJob(path, flags, limit, trackFd); // job.ondone = (err, buffer, fd, size, closeErr) => {...}; job.run(path); // Exactly one of these outcomes is reported: // err -- open/fstat/read failed (any fd opened here was closed); -// fd >= 0, size -- file is larger than `limit`: caller owns fd now (it is -// registered as an unmanaged fd iff trackFd, i.e. when the -// caller will close it through fs.close() rather than a -// FileHandle); +// fd >= 0, size -- file is larger than `limit`, or not a regular file and +// size is 0: caller owns fd now (it is registered as an +// unmanaged fd iff trackFd, i.e. when the caller will +// close it through fs.close() rather than a FileHandle); // buffer -- the whole content; closeErr set if only close() failed. class ReadFileJob final : public AsyncWrap, public ThreadPoolWork { public: @@ -3689,8 +3690,9 @@ class ReadFileJob final : public AsyncWrap, public ThreadPoolWork { const uint64_t size = regular ? static_cast(st->st_size) : 0; uv_fs_req_cleanup(&req); - if (size > limit_) { - // Too large to read in one go here: hand the fd back. + if (size > limit_ || !regular) { + // Too large to read in one go here, or possibly endless: hand the fd + // back. fd_ = fd; size_ = size; return; diff --git a/test/parallel/test-fs-readfile-fifo-abort.js b/test/parallel/test-fs-readfile-fifo-abort.js new file mode 100644 index 000000000000..d22f457a34c0 --- /dev/null +++ b/test/parallel/test-fs-readfile-fifo-abort.js @@ -0,0 +1,48 @@ +'use strict'; +// Env: UV_THREADPOOL_SIZE=2 +// fs.readFile() and fs.promises.readFile() stay abortable on a FIFO whose +// writer stays open, so the read never reaches EOF. +// +// The reader's open() holds a thread pool thread until a writer opens the +// FIFO, and the writer's open() needs a second one. +const common = require('../common'); +if (common.isWindows) + common.skip('no mkfifo on Windows'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const { execFileSync } = require('child_process'); +const fs = require('fs'); +const { promisify } = require('util'); + +tmpdir.refresh(); + +async function testAbort(name, read) { + const fifo = tmpdir.resolve(`${name}.fifo`); + execFileSync('mkfifo', [fifo]); + const ac = new AbortController(); + const settled = assert.rejects(read(fifo, { signal: ac.signal }), { name: 'AbortError' }); + const writer = await fs.promises.open(fifo, 'w'); + ac.abort(); + // A reader that honors the abort closes the FIFO after at most one more + // read, and the next write fails with EPIPE. One that ignores it keeps + // draining the FIFO until the writer closes. + let closedByReader = false; + const chunk = Buffer.alloc(1024); + for (let i = 0; i < 1000; i++) { + try { + await writer.write(chunk); + } catch (err) { + assert.strictEqual(err.code, 'EPIPE'); + closedByReader = true; + break; + } + } + await writer.close(); + assert.ok(closedByReader, `${name} kept reading after the abort`); + await settled; +} + +(async () => { + await testAbort('callback', promisify(fs.readFile)); + await testAbort('promises', fs.promises.readFile); +})().then(common.mustCall());