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

fix(ext/node): accept file descriptor in fs.readFile(Sync) #27252

Merged
merged 2 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
26 changes: 17 additions & 9 deletions ext/node/polyfills/_fs/_fs_readFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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") {
Expand Down
1 change: 1 addition & 0 deletions tests/node_compat/config.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 1 addition & 2 deletions tests/node_compat/runner/TODO.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<!-- deno-fmt-ignore-file -->
# 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.

Expand Down Expand Up @@ -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)
Expand Down
101 changes: 101 additions & 0 deletions tests/node_compat/test/parallel/test-fs-readfile-fd.js
Original file line number Diff line number Diff line change
@@ -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);
}));
}));
}));
}
}
Loading