Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: enable chunked reading for large files in readFileHandle #56022

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ const {
aggregateTwoErrors,
codes: {
ERR_ACCESS_DENIED,
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
},
} = require('internal/errors');
Expand All @@ -101,7 +100,6 @@ const {
} = require('internal/util');
const {
constants: {
kIoMaxLength,
kMaxUserId,
},
copyObject,
Expand Down Expand Up @@ -321,11 +319,6 @@ function readFileAfterStat(err, stats) {
// stringify operations vs multiple C++/JS boundary crossings).
const size = context.size = isFileType(stats, S_IFREG) ? stats[8] : 0;

if (size > kIoMaxLength) {
err = new ERR_FS_FILE_TOO_LARGE(size);
return context.close(err);
}

try {
if (size === 0) {
// TODO(BridgeAR): If an encoding is set, use the StringDecoder to concat
Expand Down Expand Up @@ -402,9 +395,6 @@ function tryCreateBuffer(size, fd, isUserFd) {
let threw = true;
let buffer;
try {
if (size > kIoMaxLength) {
throw new ERR_FS_FILE_TOO_LARGE(size);
}
buffer = Buffer.allocUnsafe(size);
threw = false;
} finally {
Expand Down
5 changes: 0 additions & 5 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ const {
aggregateTwoErrors,
codes: {
ERR_ACCESS_DENIED,
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_STATE,
ERR_METHOD_NOT_IMPLEMENTED,
Expand All @@ -49,7 +48,6 @@ const { isArrayBufferView } = require('internal/util/types');

const {
constants: {
kIoMaxLength,
kMaxUserId,
kReadFileBufferLength,
kReadFileUnknownBufferLength,
Expand Down Expand Up @@ -534,9 +532,6 @@ async function readFileHandle(filehandle, options) {
length = kReadFileUnknownBufferLength;
}

if (size > kIoMaxLength)
throw new ERR_FS_FILE_TOO_LARGE(size);

let totalRead = 0;
const noSize = size === 0;
let buffer = Buffer.allocUnsafeSlow(length);
Expand Down
55 changes: 28 additions & 27 deletions test/parallel/test-fs-promises-file-handle-readFile.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ const common = require('../common');
// The following tests validate base functionality for the fs.promises
// FileHandle.readFile method.

const fs = require('fs');
const fs = require('node:fs');
const {
open,
readFile,
writeFile,
truncate,
} = fs.promises;
const path = require('path');
const path = require('node:path');
const os = require('node:os');
const tmpdir = require('../common/tmpdir');
const tick = require('../common/tick');
const assert = require('assert');
const assert = require('node:assert');
const tmpDir = tmpdir.path;

tmpdir.refresh();
Expand All @@ -35,6 +35,29 @@ async function validateReadFile() {
await fileHandle.close();
}

async function validateLargeFileSupport() {
const LARGE_FILE_SIZE = 3 * 1024 * 1024 * 1024; // 3 GiB
const FILE_PATH = path.join(os.tmpdir(), 'large-virtual-file.bin');

function createVirtualLargeFile() {
return Buffer.alloc(LARGE_FILE_SIZE, 'A');
}

const virtualFile = createVirtualLargeFile();

await writeFile(FILE_PATH, virtualFile);

const buffer = await readFile(FILE_PATH);

assert.strictEqual(
buffer.length,
LARGE_FILE_SIZE,
`Expected file size to be ${LARGE_FILE_SIZE}, but got ${buffer.length}`
);

await fs.promises.unlink(FILE_PATH);
}

async function validateReadFileProc() {
// Test to make sure reading a file under the /proc directory works. Adapted
// from test-fs-read-file-sync-hostname.js.
Expand Down Expand Up @@ -100,32 +123,10 @@ async function doReadAndCancel() {

await fileHandle.close();
}

// Validate file size is within range for reading
{
// Variable taken from https://github.com/nodejs/node/blob/1377163f3351/lib/internal/fs/promises.js#L5
const kIoMaxLength = 2 ** 31 - 1;

if (!tmpdir.hasEnoughSpace(kIoMaxLength)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should probably be kept, no matter that we do not check for the error but for the file being read.

// truncate() will fail with ENOSPC if there is not enough space.
common.printSkipMessage(`Not enough space in ${tmpDir}`);
} else {
const newFile = path.resolve(tmpDir, 'dogs-running3.txt');
await writeFile(newFile, Buffer.from('0'));
await truncate(newFile, kIoMaxLength + 1);

const fileHandle = await open(newFile, 'r');

await assert.rejects(fileHandle.readFile(), {
name: 'RangeError',
code: 'ERR_FS_FILE_TOO_LARGE'
});
await fileHandle.close();
}
}
}

validateReadFile()
.then(validateLargeFileSupport)
.then(validateReadFileProc)
.then(doReadAndCancel)
.then(common.mustCall());
30 changes: 13 additions & 17 deletions test/parallel/test-fs-readfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const common = require('../common');

const tmpdir = require('../../test/common/tmpdir');
const assert = require('assert');
const path = require('node:path');
const fs = require('fs');

const prefix = `.removeme-fs-readfile-${process.pid}`;
Expand Down Expand Up @@ -52,26 +53,21 @@ for (const e of fileInfo) {
}));
}

// readFile() and readFileSync() should fail if the file is too big.
// Test to verify that readFile() and readFileSync() can handle large files
{
const kIoMaxLength = 2 ** 31 - 1;
const kLargeFileSize = 3 * 1024 * 1024 * 1024; // 3 GiB

if (!tmpdir.hasEnoughSpace(kIoMaxLength)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe removing this is actually a mistake :)

// truncateSync() will fail with ENOSPC if there is not enough space.
common.printSkipMessage(`Not enough space in ${tmpdir.path}`);
} else {
const file = tmpdir.resolve(`${prefix}-too-large.txt`);
fs.writeFileSync(file, Buffer.from('0'));
fs.truncateSync(file, kIoMaxLength + 1);
const file = path.join(tmpdir.path, 'temp-large-file.txt');
fs.writeFileSync(file, Buffer.alloc(1024));
fs.truncateSync(file, kLargeFileSize);

fs.readFile(file, common.expectsError({
code: 'ERR_FS_FILE_TOO_LARGE',
name: 'RangeError',
}));
assert.throws(() => {
fs.readFileSync(file);
}, { code: 'ERR_FS_FILE_TOO_LARGE', name: 'RangeError' });
}
fs.readFile(file, (err, data) => {
if (err) {
console.error('Error reading file:', err);
} else {
console.log('File read successfully:', data.length);
}
});
}

{
Expand Down
Loading