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());