From c706f5bd167d8e36c81d436fd7f3d32292e39cad Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 9 Aug 2019 09:01:43 +0200 Subject: [PATCH 1/5] fs: runtime deprecate file stream open --- doc/api/deprecations.md | 18 +++++ lib/internal/fs/streams.js | 65 +++++++++++++------ .../test-fs-read-stream-patch-open.js | 15 +++++ .../test-fs-write-stream-patch-open.js | 17 +++++ 4 files changed, 94 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 46d2467c7234b8..3480cbd9e2cdf5 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2518,6 +2518,22 @@ Type: Documentation-only Prefer [`response.socket`][] over [`response.connection`] and [`request.socket`][] over [`request.connection`]. + +### 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 the constructor or by passing a file descriptor in options. + +[`--http-parser=legacy`]: cli.html#cli_http_parser_library [`--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 @@ -2528,10 +2544,12 @@ Prefer [`response.socket`][] over [`response.connection`] and [`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 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..7e5931207b1a47 --- /dev/null +++ b/test/parallel/test-fs-write-stream-patch-open.js @@ -0,0 +1,17 @@ +'use strict'; +const common = require('../common'); +const fs = require('fs'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +common.expectWarning( + 'DeprecationWarning', + 'WriteStream.prototype.open() is deprecated', 'DEP0XXX'); +const s = fs.createWriteStream(`${tmpdir.path}/out`); +s.open(); +s.destroy(); + +// Allow overriding open(). +fs.WriteStream.prototype.open = common.mustCall(); +fs.createWriteStream('asd'); From f0e3c4d481d3ad1f65b796424e9acba55fdbd935 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 20 Aug 2019 22:55:04 +0200 Subject: [PATCH 2/5] Update test/parallel/test-fs-write-stream-patch-open.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: João Reis --- .../test-fs-write-stream-patch-open.js | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-fs-write-stream-patch-open.js b/test/parallel/test-fs-write-stream-patch-open.js index 7e5931207b1a47..31d24963ea31a6 100644 --- a/test/parallel/test-fs-write-stream-patch-open.js +++ b/test/parallel/test-fs-write-stream-patch-open.js @@ -3,7 +3,25 @@ const common = require('../common'); const fs = require('fs'); const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); + +// 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', From ce0ccdaa493b314225a26e3f161b42ab9a83a86f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 20 Aug 2019 22:55:16 +0200 Subject: [PATCH 3/5] Update test/parallel/test-fs-write-stream-patch-open.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: João Reis --- test/parallel/test-fs-write-stream-patch-open.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-fs-write-stream-patch-open.js b/test/parallel/test-fs-write-stream-patch-open.js index 31d24963ea31a6..1c82e80213f2fa 100644 --- a/test/parallel/test-fs-write-stream-patch-open.js +++ b/test/parallel/test-fs-write-stream-patch-open.js @@ -28,7 +28,6 @@ common.expectWarning( 'WriteStream.prototype.open() is deprecated', 'DEP0XXX'); const s = fs.createWriteStream(`${tmpdir.path}/out`); s.open(); -s.destroy(); // Allow overriding open(). fs.WriteStream.prototype.open = common.mustCall(); From 72453854149420cfad1fc2a6cf2d4f1ad6bb14d9 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 19 Sep 2019 21:22:41 +0200 Subject: [PATCH 4/5] fixup --- doc/api/deprecations.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 3480cbd9e2cdf5..1cf7da112a645e 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2531,7 +2531,8 @@ 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 the constructor or by passing a file descriptor in options. +opened through their corresponding factory methods or by passing a file +descriptor in options. [`--http-parser=legacy`]: cli.html#cli_http_parser_library [`--pending-deprecation`]: cli.html#cli_pending_deprecation From f87c677bde64460f7ad2da02404c39cfee7bdb58 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 19 Sep 2019 21:24:11 +0200 Subject: [PATCH 5/5] fixup --- doc/api/deprecations.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 1cf7da112a645e..2307333cf1369a 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2531,10 +2531,9 @@ 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 or by passing a file -descriptor in options. +opened through their corresponding factory methods [`fs.createWriteStream()`][] +and [`fs.createReadStream()`][]) or by passing a file descriptor in options. -[`--http-parser=legacy`]: cli.html#cli_http_parser_library [`--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 @@ -2573,6 +2572,8 @@ descriptor in options. [`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