Skip to content

Commit 0c5be92

Browse files
christianaurichzmaduh95
authored andcommitted
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 <christian.aurichzm@gmail.com> PR-URL: #66940 Refs: #65327 Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent dc4aa75 commit 0c5be92

4 files changed

Lines changed: 69 additions & 17 deletions

File tree

‎lib/fs.js‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -452,10 +452,11 @@ function readFile(path, options, callback) {
452452
const flagsNumber = stringToFlags(options.flag, 'options.flag');
453453
path = getValidatedPath(path);
454454
if (options.buffer === undefined) {
455-
// Open + fstat + read + close in one thread pool round trip for files of
456-
// up to one chunk; larger files come back as an open fd + size and take
457-
// the chunked reader below (readFileAfterOneShot). `true`: a handed-back
458-
// fd will be closed through fs.close(), so track it as unmanaged.
455+
// Open + fstat + read + close in one thread pool round trip for regular
456+
// files of up to one chunk; larger and non-regular files (size 0) come
457+
// back as an open fd + size and take the chunked reader below
458+
// (readFileAfterOneShot). `true`: a handed-back fd will be closed through
459+
// fs.close(), so track it as unmanaged.
459460
const job = new ReadFileJob(path, flagsNumber, kReadFileBufferLength, true);
460461
job.context = context;
461462
job.ondone = readFileAfterOneShot;
@@ -480,7 +481,8 @@ function readFileAfterOneShot(err, buffer, fd, size, closeErr) {
480481
}
481482
if (fd !== -1) {
482483
// (context.read() below performs the abort check for this case.)
483-
// Larger than one chunk: continue exactly like after open + fstat.
484+
// Larger than one chunk or not a regular file: continue exactly like
485+
// after open + fstat.
484486
context.fd = fd;
485487
context.size = size;
486488
if (size > kIoMaxLength) {

‎lib/internal/fs/promises.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2305,9 +2305,9 @@ async function readFile(path, options) {
23052305
checkAborted(options.signal);
23062306

23072307
if (options.buffer === undefined && vfsState.handlers === null) {
2308-
// Open + fstat + read + close in one thread pool round trip for files of
2309-
// up to one chunk; larger files come back as an open fd + size and are
2310-
// read by readFileHandle() as before.
2308+
// Open + fstat + read + close in one thread pool round trip for regular
2309+
// files of up to one chunk; larger and non-regular files (size 0) come
2310+
// back as an open fd + size and are read by readFileHandle() as before.
23112311
path = getValidatedPath(path);
23122312
const { 0: buffer, 1: fd, 2: size } = await readFileInOneRoundTrip(path, stringToFlags(flag));
23132313
if (fd === -1) {

‎src/node_file.cc‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3587,18 +3587,19 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
35873587
// read + close -- instead of one round trip per step, which is what dominates
35883588
// fs.readFile() for the typical small file and multiplies thread pool
35893589
// contention when many files are read at once. Files whose size exceeds
3590-
// `limit` are not read here: the job hands the open fd and the size back so
3591-
// that the caller continues with the chunked reader (which stays fair and
3592-
// abortable for large files); it then only saved the fstat() round trip.
3590+
// `limit`, and files that are not regular files (a FIFO or a device may never
3591+
// reach EOF), are not read here: the job hands the open fd and the size back
3592+
// so that the caller continues with the chunked reader (which stays fair and
3593+
// abortable between chunks); it then only saved the fstat() round trip.
35933594
//
35943595
// JS: const job = new ReadFileJob(path, flags, limit, trackFd);
35953596
// job.ondone = (err, buffer, fd, size, closeErr) => {...}; job.run(path);
35963597
// Exactly one of these outcomes is reported:
35973598
// err -- open/fstat/read failed (any fd opened here was closed);
3598-
// fd >= 0, size -- file is larger than `limit`: caller owns fd now (it is
3599-
// registered as an unmanaged fd iff trackFd, i.e. when the
3600-
// caller will close it through fs.close() rather than a
3601-
// FileHandle);
3599+
// fd >= 0, size -- file is larger than `limit`, or not a regular file and
3600+
// size is 0: caller owns fd now (it is registered as an
3601+
// unmanaged fd iff trackFd, i.e. when the caller will
3602+
// close it through fs.close() rather than a FileHandle);
36023603
// buffer -- the whole content; closeErr set if only close() failed.
36033604
class ReadFileJob final : public AsyncWrap, public ThreadPoolWork {
36043605
public:
@@ -3663,8 +3664,9 @@ class ReadFileJob final : public AsyncWrap, public ThreadPoolWork {
36633664
const uint64_t size = regular ? static_cast<uint64_t>(st->st_size) : 0;
36643665
uv_fs_req_cleanup(&req);
36653666

3666-
if (size > limit_) {
3667-
// Too large to read in one go here: hand the fd back.
3667+
if (size > limit_ || !regular) {
3668+
// Too large to read in one go here, or possibly endless: hand the fd
3669+
// back.
36683670
fd_ = fd;
36693671
size_ = size;
36703672
return;
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
'use strict';
2+
// Env: UV_THREADPOOL_SIZE=2
3+
// fs.readFile() and fs.promises.readFile() stay abortable on a FIFO whose
4+
// writer stays open, so the read never reaches EOF.
5+
//
6+
// The reader's open() holds a thread pool thread until a writer opens the
7+
// FIFO, and the writer's open() needs a second one.
8+
const common = require('../common');
9+
if (common.isWindows)
10+
common.skip('no mkfifo on Windows');
11+
const tmpdir = require('../common/tmpdir');
12+
const assert = require('assert');
13+
const { execFileSync } = require('child_process');
14+
const fs = require('fs');
15+
const { promisify } = require('util');
16+
17+
tmpdir.refresh();
18+
19+
async function testAbort(name, read) {
20+
const fifo = tmpdir.resolve(`${name}.fifo`);
21+
execFileSync('mkfifo', [fifo]);
22+
const ac = new AbortController();
23+
const settled = assert.rejects(read(fifo, { signal: ac.signal }), { name: 'AbortError' });
24+
const writer = await fs.promises.open(fifo, 'w');
25+
ac.abort();
26+
// A reader that honors the abort closes the FIFO after at most one more
27+
// read, and the next write fails with EPIPE. One that ignores it keeps
28+
// draining the FIFO until the writer closes.
29+
let closedByReader = false;
30+
const chunk = Buffer.alloc(1024);
31+
for (let i = 0; i < 1000; i++) {
32+
try {
33+
await writer.write(chunk);
34+
} catch (err) {
35+
assert.strictEqual(err.code, 'EPIPE');
36+
closedByReader = true;
37+
break;
38+
}
39+
}
40+
await writer.close();
41+
assert.ok(closedByReader, `${name} kept reading after the abort`);
42+
await settled;
43+
}
44+
45+
(async () => {
46+
await testAbort('callback', promisify(fs.readFile));
47+
await testAbort('promises', fs.promises.readFile);
48+
})().then(common.mustCall());

0 commit comments

Comments
 (0)