From 59fff925beb56efe09b66f62dee6acb6900d9d8b Mon Sep 17 00:00:00 2001
From: Antoine du Hamel <duhamelantoine1995@gmail.com>
Date: Mon, 6 Sep 2021 11:33:10 +0200
Subject: [PATCH] fs: make `open` and `close` stream override optional when
 unused

When using `createReadStream` or `createWriteStream` with a specific
file descriptor or `FileHandle` instead of a path, `open` method is not
used, there is no point in forcing users to provide it.
When using `createReadStream` or `createWriteStream` with  `autoClose`
set to false, `close` method is not used, there is no point in forcing
users to provide it.

PR-URL: https://github.com/nodejs/node/pull/40013
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/fs.md              |  22 ++++++-
 lib/internal/fs/streams.js | 114 +++++++++++++++++++------------------
 2 files changed, 77 insertions(+), 59 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 0cb93871b9924e..37fb8143497ced 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -1887,6 +1887,12 @@ behavior is similar to `cp dir1/ dir2/`.
 <!-- YAML
 added: v0.1.31
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40013
+    description: The `fs` option does not need `open` method if an `fd` was provided.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40013
+    description: The `fs` option does not need `close` method if `autoClose` is `false`.
   - version:
      - v15.4.0
     pr-url: https://github.com/nodejs/node/pull/35922
@@ -1962,7 +1968,9 @@ destroyed, like most `Readable` streams.  Set the `emitClose` option to
 
 By providing the `fs` option, it is possible to override the corresponding `fs`
 implementations for `open`, `read`, and `close`. When providing the `fs` option,
-overrides for `open`, `read`, and `close` are required.
+an override for `read` is required. If no `fd` is provided, an override for
+`open` is also required. If `autoClose` is `true`, an override for `close` is
+also required.
 
 ```mjs
 import { createReadStream } from 'fs';
@@ -2004,6 +2012,12 @@ If `options` is a string, then it specifies the encoding.
 <!-- YAML
 added: v0.1.31
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40013
+    description: The `fs` option does not need `open` method if an `fd` was provided.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40013
+    description: The `fs` option does not need `close` method if `autoClose` is `false`.
   - version:
      - v15.4.0
     pr-url: https://github.com/nodejs/node/pull/35922
@@ -2067,8 +2081,10 @@ destroyed, like most `Writable` streams.  Set the `emitClose` option to
 By providing the `fs` option it is possible to override the corresponding `fs`
 implementations for `open`, `write`, `writev` and `close`. Overriding `write()`
 without `writev()` can reduce performance as some optimizations (`_writev()`)
-will be disabled. When providing the `fs` option,  overrides for `open`,
-`close`, and at least one of `write` and `writev` are required.
+will be disabled. When providing the `fs` option, overrides for at least one of
+`write` and `writev` are required. If no `fd` option is supplied, an override
+for `open` is also required. If `autoClose` is `true`, an override for `close`
+is also required.
 
 Like {fs.ReadStream}, if `fd` is specified, {fs.WriteStream} will ignore the
 `path` argument and will use the specified file descriptor. This means that no
diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js
index b388bad80c74a5..5e9e338ab1fcbd 100644
--- a/lib/internal/fs/streams.js
+++ b/lib/internal/fs/streams.js
@@ -120,29 +120,29 @@ function close(stream, err, cb) {
 }
 
 function importFd(stream, options) {
-  stream.fd = null;
-  if (options.fd != null) {
-    if (typeof options.fd === 'number') {
-      // When fd is a raw descriptor, we must keep our fingers crossed
-      // that the descriptor won't get closed, or worse, replaced with
-      // another one
-      // https://github.com/nodejs/node/issues/35862
-      stream.fd = options.fd;
-    } else if (typeof options.fd === 'object' &&
-      options.fd instanceof FileHandle) {
-      // When fd is a FileHandle we can listen for 'close' events
-      if (options.fs)
-        // FileHandle is not supported with custom fs operations
-        throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs');
-      stream[kHandle] = options.fd;
-      stream.fd = options.fd.fd;
-      stream[kFs] = FileHandleOperations(stream[kHandle]);
-      stream[kHandle][kRef]();
-      options.fd.on('close', FunctionPrototypeBind(stream.close, stream));
-    } else
-      throw ERR_INVALID_ARG_TYPE('options.fd',
-                                 ['number', 'FileHandle'], options.fd);
+  if (typeof options.fd === 'number') {
+    // When fd is a raw descriptor, we must keep our fingers crossed
+    // that the descriptor won't get closed, or worse, replaced with
+    // another one
+    // https://github.com/nodejs/node/issues/35862
+    stream[kFs] = options.fs || fs;
+    return options.fd;
+  } else if (typeof options.fd === 'object' &&
+             options.fd instanceof FileHandle) {
+    // When fd is a FileHandle we can listen for 'close' events
+    if (options.fs) {
+      // FileHandle is not supported with custom fs operations
+      throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs');
+    }
+    stream[kHandle] = options.fd;
+    stream[kFs] = FileHandleOperations(stream[kHandle]);
+    stream[kHandle][kRef]();
+    options.fd.on('close', FunctionPrototypeBind(stream.close, stream));
+    return options.fd.fd;
   }
+
+  throw ERR_INVALID_ARG_TYPE('options.fd',
+                             ['number', 'FileHandle'], options.fd);
 }
 
 function ReadStream(path, options) {
@@ -158,21 +158,29 @@ function ReadStream(path, options) {
     options.autoDestroy = false;
   }
 
-  this[kFs] = options.fs || fs;
+  if (options.fd == null) {
+    this.fd = null;
+    this[kFs] = options.fs || fs;
+    validateFunction(this[kFs].open, 'options.fs.open');
 
-  validateFunction(this[kFs].open, 'options.fs.open');
-  validateFunction(this[kFs].read, 'options.fs.read');
-  validateFunction(this[kFs].close, 'options.fs.close');
+    // Path will be ignored when fd is specified, so it can be falsy
+    this.path = toPathIfFileURL(path);
+    this.flags = options.flags === undefined ? 'r' : options.flags;
+    this.mode = options.mode === undefined ? 0o666 : options.mode;
+
+    validatePath(this.path);
+  } else {
+    this.fd = getValidatedFd(importFd(this, options));
+  }
 
   options.autoDestroy = options.autoClose === undefined ?
     true : options.autoClose;
 
-  // Path will be ignored when fd is specified, so it can be falsy
-  this.path = toPathIfFileURL(path);
-  this.flags = options.flags === undefined ? 'r' : options.flags;
-  this.mode = options.mode === undefined ? 0o666 : options.mode;
+  validateFunction(this[kFs].read, 'options.fs.read');
 
-  importFd(this, options);
+  if (options.autoDestroy) {
+    validateFunction(this[kFs].close, 'options.fs.close');
+  }
 
   this.start = options.start;
   this.end = options.end;
@@ -187,12 +195,6 @@ function ReadStream(path, options) {
     this.pos = this.start;
   }
 
-  // If fd has been set, validate, otherwise validate path.
-  if (this.fd != null) {
-    this.fd = getValidatedFd(this.fd);
-  } else {
-    validatePath(this.path);
-  }
 
   if (this.end === undefined) {
     this.end = Infinity;
@@ -310,9 +312,23 @@ function WriteStream(path, options) {
   // Only buffers are supported.
   options.decodeStrings = true;
 
-  this[kFs] = options.fs || fs;
+  if (options.fd == null) {
+    this.fd = null;
+    this[kFs] = options.fs || fs;
+    validateFunction(this[kFs].open, 'options.fs.open');
+
+    // Path will be ignored when fd is specified, so it can be falsy
+    this.path = toPathIfFileURL(path);
+    this.flags = options.flags === undefined ? 'w' : options.flags;
+    this.mode = options.mode === undefined ? 0o666 : options.mode;
+
+    validatePath(this.path);
+  } else {
+    this.fd = getValidatedFd(importFd(this, options));
+  }
 
-  validateFunction(this[kFs].open, 'options.fs.open');
+  options.autoDestroy = options.autoClose === undefined ?
+    true : options.autoClose;
 
   if (!this[kFs].write && !this[kFs].writev) {
     throw new ERR_INVALID_ARG_TYPE('options.fs.write', 'function',
@@ -327,7 +343,9 @@ function WriteStream(path, options) {
     validateFunction(this[kFs].writev, 'options.fs.writev');
   }
 
-  validateFunction(this[kFs].close, 'options.fs.close');
+  if (options.autoDestroy) {
+    validateFunction(this[kFs].close, 'options.fs.close');
+  }
 
   // It's enough to override either, in which case only one will be used.
   if (!this[kFs].write) {
@@ -337,28 +355,12 @@ function WriteStream(path, options) {
     this._writev = null;
   }
 
-  options.autoDestroy = options.autoClose === undefined ?
-    true : options.autoClose;
-
-  // Path will be ignored when fd is specified, so it can be falsy
-  this.path = toPathIfFileURL(path);
-  this.flags = options.flags === undefined ? 'w' : options.flags;
-  this.mode = options.mode === undefined ? 0o666 : options.mode;
-
-  importFd(this, options);
-
   this.start = options.start;
   this.pos = undefined;
   this.bytesWritten = 0;
   this.closed = false;
   this[kIsPerformingIO] = false;
 
-  // If fd has been set, validate, otherwise validate path.
-  if (this.fd != null) {
-    this.fd = getValidatedFd(this.fd);
-  } else {
-    validatePath(this.path);
-  }
 
   if (this.start !== undefined) {
     validateInteger(this.start, 'start', 0);