From 07737b03bc50e4b77c9a287d6caad6eaa40759d8 Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Fri, 6 Dec 2024 20:18:08 +0900 Subject: [PATCH] fix(ext/node): accept file descriptor in fs.readFile(Sync) (#27252) closes #27123 --- ext/node/polyfills/_fs/_fs_readFile.ts | 26 +++-- tests/node_compat/config.jsonc | 1 + tests/node_compat/runner/TODO.md | 3 +- .../test/parallel/test-fs-readfile-fd.js | 101 ++++++++++++++++++ 4 files changed, 120 insertions(+), 11 deletions(-) create mode 100644 tests/node_compat/test/parallel/test-fs-readfile-fd.js diff --git a/ext/node/polyfills/_fs/_fs_readFile.ts b/ext/node/polyfills/_fs/_fs_readFile.ts index 029e57c502..b1bc53675d 100644 --- a/ext/node/polyfills/_fs/_fs_readFile.ts +++ b/ext/node/polyfills/_fs/_fs_readFile.ts @@ -10,7 +10,7 @@ import { TextOptionsArgument, } from "ext:deno_node/_fs/_fs_common.ts"; import { Buffer } from "node:buffer"; -import { readAll } from "ext:deno_io/12_io.js"; +import { readAll, readAllSync } from "ext:deno_io/12_io.js"; import { FileHandle } from "ext:deno_node/internal/fs/handle.ts"; import { pathFromURL } from "ext:deno_web/00_infra.js"; import { @@ -39,7 +39,7 @@ type TextCallback = (err: Error | null, data?: string) => void; type BinaryCallback = (err: Error | null, data?: Buffer) => void; type GenericCallback = (err: Error | null, data?: string | Buffer) => void; type Callback = TextCallback | BinaryCallback | GenericCallback; -type Path = string | URL | FileHandle; +type Path = string | URL | FileHandle | number; export function readFile( path: Path, @@ -76,6 +76,9 @@ export function readFile( if (path instanceof FileHandle) { const fsFile = new FsFile(path.fd, Symbol.for("Deno.internal.FsFile")); p = readAll(fsFile); + } else if (typeof path === "number") { + const fsFile = new FsFile(path, Symbol.for("Deno.internal.FsFile")); + p = readAll(fsFile); } else { p = Deno.readFile(path); } @@ -106,23 +109,28 @@ export function readFilePromise( } export function readFileSync( - path: string | URL, + path: string | URL | number, opt: TextOptionsArgument, ): string; export function readFileSync( - path: string | URL, + path: string | URL | number, opt?: BinaryOptionsArgument, ): Buffer; export function readFileSync( - path: string | URL, + path: string | URL | number, opt?: FileOptionsArgument, ): string | Buffer { path = path instanceof URL ? pathFromURL(path) : path; let data; - try { - data = Deno.readFileSync(path); - } catch (err) { - throw denoErrorToNodeError(err, { path, syscall: "open" }); + if (typeof path === "number") { + const fsFile = new FsFile(path, Symbol.for("Deno.internal.FsFile")); + data = readAllSync(fsFile); + } else { + try { + data = Deno.readFileSync(path); + } catch (err) { + throw denoErrorToNodeError(err, { path, syscall: "open" }); + } } const encoding = getEncoding(opt); if (encoding && encoding !== "binary") { diff --git a/tests/node_compat/config.jsonc b/tests/node_compat/config.jsonc index 04cb4e6e2d..e7d4e9b944 100644 --- a/tests/node_compat/config.jsonc +++ b/tests/node_compat/config.jsonc @@ -503,6 +503,7 @@ "test-fs-readdir-stack-overflow.js", "test-fs-readdir.js", "test-fs-readfile-empty.js", + "test-fs-readfile-fd.js", "test-fs-readfile-unlink.js", "test-fs-readfile-zero-byte-liar.js", "test-fs-readfilesync-enoent.js", diff --git a/tests/node_compat/runner/TODO.md b/tests/node_compat/runner/TODO.md index d33231fec7..994ae84b89 100644 --- a/tests/node_compat/runner/TODO.md +++ b/tests/node_compat/runner/TODO.md @@ -1,7 +1,7 @@ # Remaining Node Tests -1163 tests out of 3681 have been ported from Node 20.11.1 (31.59% ported, 68.92% remaining). +1164 tests out of 3681 have been ported from Node 20.11.1 (31.62% ported, 68.89% remaining). NOTE: This file should not be manually edited. Please edit `tests/node_compat/config.json` and run `deno task setup` in `tests/node_compat/runner` dir instead. @@ -691,7 +691,6 @@ NOTE: This file should not be manually edited. Please edit `tests/node_compat/co - [parallel/test-fs-readdir-types.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readdir-types.js) - [parallel/test-fs-readdir-ucs2.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readdir-ucs2.js) - [parallel/test-fs-readfile-error.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readfile-error.js) -- [parallel/test-fs-readfile-fd.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readfile-fd.js) - [parallel/test-fs-readfile-flags.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readfile-flags.js) - [parallel/test-fs-readfile-pipe-large.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readfile-pipe-large.js) - [parallel/test-fs-readfile-pipe.js](https://github.com/nodejs/node/tree/v20.11.1/test/parallel/test-fs-readfile-pipe.js) diff --git a/tests/node_compat/test/parallel/test-fs-readfile-fd.js b/tests/node_compat/test/parallel/test-fs-readfile-fd.js new file mode 100644 index 0000000000..7edfd1d6a9 --- /dev/null +++ b/tests/node_compat/test/parallel/test-fs-readfile-fd.js @@ -0,0 +1,101 @@ +// deno-fmt-ignore-file +// deno-lint-ignore-file + +// Copyright Joyent and Node contributors. All rights reserved. MIT license. +// Taken from Node 20.11.1 +// This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually. + +'use strict'; +const common = require('../common'); + +// Test fs.readFile using a file descriptor. + +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const fs = require('fs'); +const fn = fixtures.path('empty.txt'); +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +tempFd(function(fd, close) { + fs.readFile(fd, function(err, data) { + assert.ok(data); + close(); + }); +}); + +tempFd(function(fd, close) { + fs.readFile(fd, 'utf8', function(err, data) { + assert.strictEqual(data, ''); + close(); + }); +}); + +tempFdSync(function(fd) { + assert.ok(fs.readFileSync(fd)); +}); + +tempFdSync(function(fd) { + assert.strictEqual(fs.readFileSync(fd, 'utf8'), ''); +}); + +function tempFd(callback) { + fs.open(fn, 'r', function(err, fd) { + assert.ifError(err); + callback(fd, function() { + fs.close(fd, function(err) { + assert.ifError(err); + }); + }); + }); +} + +function tempFdSync(callback) { + const fd = fs.openSync(fn, 'r'); + callback(fd); + fs.closeSync(fd); +} + +{ + // This test makes sure that `readFile()` always reads from the current + // position of the file, instead of reading from the beginning of the file, + // when used with file descriptors. + + const filename = tmpdir.resolve('test.txt'); + fs.writeFileSync(filename, 'Hello World'); + + { + // Tests the fs.readFileSync(). + const fd = fs.openSync(filename, 'r'); + + // Read only five bytes, so that the position moves to five. + const buf = Buffer.alloc(5); + assert.strictEqual(fs.readSync(fd, buf, 0, 5), 5); + assert.strictEqual(buf.toString(), 'Hello'); + + // readFileSync() should read from position five, instead of zero. + assert.strictEqual(fs.readFileSync(fd).toString(), ' World'); + + fs.closeSync(fd); + } + + { + // Tests the fs.readFile(). + fs.open(filename, 'r', common.mustSucceed((fd) => { + const buf = Buffer.alloc(5); + + // Read only five bytes, so that the position moves to five. + fs.read(fd, buf, 0, 5, null, common.mustSucceed((bytes) => { + assert.strictEqual(bytes, 5); + assert.strictEqual(buf.toString(), 'Hello'); + + fs.readFile(fd, common.mustSucceed((data) => { + // readFile() should read from position five, instead of zero. + assert.strictEqual(data.toString(), ' World'); + + fs.closeSync(fd); + })); + })); + })); + } +}