From 773769df60ac4f2448fa88b2ece035de2512928f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 9 Aug 2019 09:01:43 +0200 Subject: [PATCH] fs: add runtime deprecate for file stream open() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/29061 Reviewed-By: Ben Noordhuis Reviewed-By: Matteo Collina Reviewed-By: Jeremiah Senkpiel Reviewed-By: João Reis Reviewed-By: Ruben Bridgewater --- doc/api/deprecations.md | 20 ++++++ lib/internal/fs/streams.js | 65 +++++++++++++------ .../test-fs-read-stream-patch-open.js | 15 +++++ .../test-fs-write-stream-patch-open.js | 34 ++++++++++ 4 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-fs-read-stream-patch-open.js create mode 100644 test/parallel/test-fs-write-stream-patch-open.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index aaec4a3c998497..8b36e0c3aa8d7c 100644 --- 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. + +### DEP0XXX: `WriteStream.open()` and `ReadStream.open()` are internal + + +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: })`]: 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 index 110ad114930fca..3674a71f92ba15 100644 --- 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 new file mode 100644 index 00000000000000..80f8888de30042 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..1c82e80213f2fa --- /dev/null +++ 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');