fs,net: emitting ready event for fs streams and network sockets in addition… #19408

sameer-coder · 2018-03-17T10:59:13Z

… to the event names they currently use.

Currently, various internal streams have different events that indicate that the
underlying resource has successfully been established.

fs streams use the open event once the file descriptor is available
net.Sockets use the connect event once the socket has been established
Http2Streams use the ready event once the underlying http/2 session is established

This commit adds ready event for fs and net sockets to standardize on emitting ready for all of these streams.

Fixes: #19304

Checklist

[x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
[x ] tests and/or benchmarks are included
[x ] commit message follows commit guidelines

Affected core subsystem(s)
fs, net

… to the event names they currently use. Currently, various internal streams have different events that indicate that the underlying resource has successfully been established. This commit adds ready event for fs and net sockets to standardize on emitting ready for all of these streams. Fixes: nodejs#19304

addaleax

Thank you for picking this up!

addaleax · 2018-03-17T11:04:45Z

lib/fs.js

@@ -1984,6 +1984,7 @@ ReadStream.prototype.open = function() {

    self.fd = fd;
    self.emit('open', fd);
+    self.emit('ready', fd);


I would omit fd here, the other streams don’t have this on the event (and it doesn’t make sense to have it for Http2Stream)

I have removed thefd for the ready event.

addaleax · 2018-03-17T11:06:14Z

test/parallel/test-fs-ready-event-stream.js

+  const pauseRes = fs.createReadStream(rangeFile);
+  pauseRes.pause();
+  pauseRes.resume();
+}


I guess a lot of these blocks are taken from another test? It might be good to reduce the file so that only the one with the ready event remains

I have removed all the extra blocks and made it a minimal testcase to only check for ready event.

sameer-coder · 2018-03-21T07:24:25Z

Thanks @addaleax I have been a bit busy so couldn't update yet. I will make the changes as per your feedback today and commit again.

removed file descriptor for ready event in fs streams and updated test case to check only for ready event. Fixes: nodejs#19304

updated test case updated test case to check for ready event on writestream Fixes: nodejs#19304

…am.js Fixes: nodejs#19304

addaleax

Thanks!

addaleax · 2018-03-21T10:55:40Z

CI: https://ci.nodejs.org/job/node-test-commit/17079/

sameer-coder · 2018-03-22T06:21:58Z

The CI failures seem unrelated to this PR.

addaleax · 2018-03-23T12:38:42Z

Landed in 1c81494 🎉

... in addition to the event names they currently use. Currently, various internal streams have different events that indicate that the underlying resource has successfully been established. This commit adds ready event for fs and net sockets to standardize on emitting ready for all of these streams. PR-URL: #19408 Fixes: #19304 Reviewed-By: Anna Henningsen <anna@addaleax.net>

sameer-coder · 2018-03-23T12:41:46Z

Thanks a lot @addaleax for your guidance ;)

lpinca · 2018-03-26T12:12:01Z

Should we add documentation for these new events?

... in addition to the event names they currently use. Currently, various internal streams have different events that indicate that the underlying resource has successfully been established. This commit adds ready event for fs and net sockets to standardize on emitting ready for all of these streams. PR-URL: #19408 Fixes: #19304 Reviewed-By: Anna Henningsen <anna@addaleax.net>

targos · 2018-04-04T14:57:26Z

It looks like this should be semver-minor. Please remove the label if I'm mistaken.

Notable changes: * deps: - Updated ICU to 61.1 (Steven R. Loomis) [#19621](#19621) Includes CLDR 33 (many new languages and data improvements). * fs: - Emit 'ready' event for `ReadStream` and `WriteStream` (Sameer Srivastava) [#19408](#19408) * n-api: - Bump version of n-api supported (Michael Dawson) [#19497](#19497) * net: - Emit 'ready' event for `Socket` (Sameer Srivastava) [#19408](#19408) * Added new collaborators - [mafintosh](https://github.com/mafintosh) Mathias Buus

... in addition to the event names they currently use. Currently, various internal streams have different events that indicate that the underlying resource has successfully been established. This commit adds ready event for fs and net sockets to standardize on emitting ready for all of these streams. PR-URL: #19408 Fixes: #19304 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. net Issues and PRs related to the net subsystem. labels Mar 17, 2018

addaleax reviewed Mar 17, 2018

View reviewed changes

sameer-coder added 4 commits March 21, 2018 14:51

fs,net: removed file descriptor for ready event and updated test case

01ee0db

removed file descriptor for ready event in fs streams and updated test case to check only for ready event. Fixes: nodejs#19304

fs,net: removed file descriptor for ready event (writestream)

e93c869

removed file descriptor for ready event in fs streams and updated test case to check only for ready event. Fixes: nodejs#19304

fs,net: updated test case to check for ready event on writestream

0778b58

updated test case updated test case to check for ready event on writestream Fixes: nodejs#19304

fs,net: Fixed linter errors in test/parallel/test-fs-ready-event-stre…

008474a

…am.js Fixes: nodejs#19304

addaleax approved these changes Mar 21, 2018

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 23, 2018

addaleax closed this Mar 23, 2018

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 4, 2018

targos mentioned this pull request Apr 4, 2018

fs,net: missing documentation for new "ready" events #19796

Closed

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MylesBorins added the land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs,net: emitting ready event for fs streams and network sockets in addition… #19408

fs,net: emitting ready event for fs streams and network sockets in addition… #19408

sameer-coder commented Mar 17, 2018

addaleax left a comment

addaleax Mar 17, 2018

sameer-coder Mar 21, 2018

addaleax Mar 17, 2018

sameer-coder Mar 21, 2018

sameer-coder commented Mar 21, 2018

addaleax left a comment

addaleax commented Mar 21, 2018

sameer-coder commented Mar 22, 2018

addaleax commented Mar 23, 2018

sameer-coder commented Mar 23, 2018

lpinca commented Mar 26, 2018

targos commented Apr 4, 2018

fs,net: emitting ready event for fs streams and network sockets in addition… #19408

fs,net: emitting ready event for fs streams and network sockets in addition… #19408

Conversation

sameer-coder commented Mar 17, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax Mar 17, 2018

Choose a reason for hiding this comment

sameer-coder Mar 21, 2018

Choose a reason for hiding this comment

addaleax Mar 17, 2018

Choose a reason for hiding this comment

sameer-coder Mar 21, 2018

Choose a reason for hiding this comment

sameer-coder commented Mar 21, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Mar 21, 2018

sameer-coder commented Mar 22, 2018

addaleax commented Mar 23, 2018

sameer-coder commented Mar 23, 2018

lpinca commented Mar 26, 2018

targos commented Apr 4, 2018