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

closes #27123

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") {

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",

tests/node_compat/runner/TODO.md

@@ -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.
 
@@ -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)

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);
+        }));
+      }));
+    }));
+  }
+}