fs: add runtime deprecate for file stream open()

ronag · Trott · commit 773769df60ac · 2019-10-12T13:13:34.000-07:00
WriteStream.open() and ReadStream.open() are undocumented internal APIs that do not make sense to use in userland. File streams should always be opened through their corresponding factory methods (fs.createWriteStream() and fs.createReadStream()) or by passing a file descriptor in options. PR-URL: #29061 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -2532,6 +2532,22 @@ Type: Documentation-only (supports [`--pending-deprecation`][])
 The `process._tickCallback` property was never documented as
 an officially supported API.
 
+<a id="DEP0XXX"></a>
+### DEP0XXX: `WriteStream.open()` and `ReadStream.open()` are internal
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/29061
+    description: Runtime deprecation.
+-->
+
+Type: Runtime
+
+[`WriteStream.open()`][] and [`ReadStream.open()`][] are undocumented internal
+APIs that do not make sense to use in userland. File streams should always be
+opened through their corresponding factory methods [`fs.createWriteStream()`][]
+and [`fs.createReadStream()`][]) or by passing a file descriptor in options.
+
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`--throw-deprecation`]: cli.html#cli_throw_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
@@ -2542,10 +2558,12 @@ an officially supported API.
 [`Decipher`]: crypto.html#crypto_class_decipher
 [`EventEmitter.listenerCount(emitter, eventName)`]: events.html#events_eventemitter_listenercount_emitter_eventname
 [`REPLServer.clearBufferedCommand()`]: repl.html#repl_replserver_clearbufferedcommand
+[`ReadStream.open()`]: fs.html#fs_class_fs_readstream
 [`Server.connections`]: net.html#net_server_connections
 [`Server.getConnections()`]: net.html#net_server_getconnections_callback
 [`Server.listen({fd: <number>})`]: net.html#net_server_listen_handle_backlog_callback
 [`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
+[`WriteStream.open()`]: fs.html#fs_class_fs_writestream
 [`assert`]: assert.html
 [`asyncResource.runInAsyncScope()`]: async_hooks.html#async_hooks_asyncresource_runinasyncscope_fn_thisarg_args
 [`child_process`]: child_process.html
@@ -2568,6 +2586,8 @@ an officially supported API.
 [`ecdh.setPublicKey()`]: crypto.html#crypto_ecdh_setpublickey_publickey_encoding
 [`emitter.listenerCount(eventName)`]: events.html#events_emitter_listenercount_eventname
 [`fs.access()`]: fs.html#fs_fs_access_path_mode_callback
+[`fs.createReadStream()`]: fs.html#fs_fs_createreadstream_path_options
+[`fs.createWriteStream()`]: fs.html#fs_fs_createwritestream_path_options
 [`fs.exists(path, callback)`]: fs.html#fs_fs_exists_path_callback
 [`fs.lchmod(path, mode, callback)`]: fs.html#fs_fs_lchmod_path_mode_callback
 [`fs.lchmodSync(path, mode)`]: fs.html#fs_fs_lchmodsync_path_mode
diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js
@@ -5,6 +5,7 @@ const { Math, Object } = primordials;
 const {
   ERR_OUT_OF_RANGE
 } = require('internal/errors').codes;
+const internalUtil = require('internal/util');
 const { validateNumber } = require('internal/validators');
 const fs = require('fs');
 const { Buffer } = require('buffer');
@@ -100,7 +101,7 @@ function ReadStream(path, options) {
   }
 
   if (typeof this.fd !== 'number')
-    this.open();
+    _openReadFs(this);
 
   this.on('end', function() {
     if (this.autoClose) {
@@ -111,23 +112,34 @@ function ReadStream(path, options) {
 Object.setPrototypeOf(ReadStream.prototype, Readable.prototype);
 Object.setPrototypeOf(ReadStream, Readable);
 
-ReadStream.prototype.open = function() {
-  fs.open(this.path, this.flags, this.mode, (er, fd) => {
+const openReadFs = internalUtil.deprecate(function() {
+  _openReadFs(this);
+}, 'ReadStream.prototype.open() is deprecated', 'DEP0XXX');
+ReadStream.prototype.open = openReadFs;
+
+function _openReadFs(stream) {
+  // Backwards compat for overriden open.
+  if (stream.open !== openReadFs) {
+    stream.open();
+    return;
+  }
+
+  fs.open(stream.path, stream.flags, stream.mode, (er, fd) => {
     if (er) {
-      if (this.autoClose) {
-        this.destroy();
+      if (stream.autoClose) {
+        stream.destroy();
       }
-      this.emit('error', er);
+      stream.emit('error', er);
       return;
     }
 
-    this.fd = fd;
-    this.emit('open', fd);
-    this.emit('ready');
+    stream.fd = fd;
+    stream.emit('open', fd);
+    stream.emit('ready');
     // Start the flow of data.
-    this.read();
+    stream.read();
   });
-};
+}
 
 ReadStream.prototype._read = function(n) {
   if (typeof this.fd !== 'number') {
@@ -266,7 +278,7 @@ function WriteStream(path, options) {
     this.setDefaultEncoding(options.encoding);
 
   if (typeof this.fd !== 'number')
-    this.open();
+    _openWriteFs(this);
 }
 Object.setPrototypeOf(WriteStream.prototype, Writable.prototype);
 Object.setPrototypeOf(WriteStream, Writable);
@@ -279,21 +291,32 @@ WriteStream.prototype._final = function(callback) {
   callback();
 };
 
-WriteStream.prototype.open = function() {
-  fs.open(this.path, this.flags, this.mode, (er, fd) => {
+const openWriteFs = internalUtil.deprecate(function() {
+  _openWriteFs(this);
+}, 'WriteStream.prototype.open() is deprecated', 'DEP0XXX');
+WriteStream.prototype.open = openWriteFs;
+
+function _openWriteFs(stream) {
+  // Backwards compat for overriden open.
+  if (stream.open !== openWriteFs) {
+    stream.open();
+    return;
+  }
+
+  fs.open(stream.path, stream.flags, stream.mode, (er, fd) => {
     if (er) {
-      if (this.autoClose) {
-        this.destroy();
+      if (stream.autoClose) {
+        stream.destroy();
       }
-      this.emit('error', er);
+      stream.emit('error', er);
       return;
     }
 
-    this.fd = fd;
-    this.emit('open', fd);
-    this.emit('ready');
+    stream.fd = fd;
+    stream.emit('open', fd);
+    stream.emit('ready');
   });
-};
+}
 
 
 WriteStream.prototype._write = function(data, encoding, cb) {
diff --git a/test/parallel/test-fs-read-stream-patch-open.js b/test/parallel/test-fs-read-stream-patch-open.js
@@ -0,0 +1,15 @@
+'use strict';
+const common = require('../common');
+const fs = require('fs');
+
+common.expectWarning(
+  'DeprecationWarning',
+  'ReadStream.prototype.open() is deprecated', 'DEP0XXX');
+const s = fs.createReadStream('asd')
+  // We don't care about errors in this test.
+  .on('error', () => {});
+s.open();
+
+// Allow overriding open().
+fs.ReadStream.prototype.open = common.mustCall();
+fs.createReadStream('asd');
diff --git a/test/parallel/test-fs-write-stream-patch-open.js b/test/parallel/test-fs-write-stream-patch-open.js
@@ -0,0 +1,34 @@
+'use strict';
+const common = require('../common');
+const fs = require('fs');
+
+const tmpdir = require('../common/tmpdir');
+
+// Run in a child process because 'out' is opened twice, blocking the tmpdir
+// and preventing cleanup.
+if (process.argv[2] !== 'child') {
+  // Parent
+  const assert = require('assert');
+  const { fork } = require('child_process');
+  tmpdir.refresh();
+
+  // Run test
+  const child = fork(__filename, ['child'], { stdio: 'inherit' });
+  child.on('exit', common.mustCall(function(code) {
+    assert.strictEqual(code, 0);
+  }));
+
+  return;
+}
+
+// Child
+
+common.expectWarning(
+  'DeprecationWarning',
+  'WriteStream.prototype.open() is deprecated', 'DEP0XXX');
+const s = fs.createWriteStream(`${tmpdir.path}/out`);
+s.open();
+
+// Allow overriding open().
+fs.WriteStream.prototype.open = common.mustCall();
+fs.createWriteStream('asd');
