lib: return this from net.Socket.end() #13481

sam-github · 2017-06-05T19:37:06Z

Checklist

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

Affected core subsystem(s)

net

cjihrig · 2017-06-05T19:40:45Z

test/parallel/test-socket-write-after-fin.js

  sock.write('hello1');
  sock.write('hello2');
  sock.write('hello3\n');
-  sock.end('THUNDERMUSCLE!');
+  sock.end('THUNDERMUSCLE!') .on('end', common.mustCall(function() {


Extra space before .on

cjihrig

LGTM but an explicit assert.strictEqual() might be better in the test since someone will likely rewrite this file in the future and lose the implicit assertion.

mscdex · 2017-06-05T19:53:53Z

LGTM once the aforementioned nits are addressed.

sam-github · 2017-06-05T20:05:13Z

Implemented suggestions, thank you.

ci: https://ci.nodejs.org/job/node-test-pull-request/8492/

jasnell · 2017-06-06T00:04:20Z

test/parallel/test-socket-write-after-fin.js

@@ -35,5 +35,6 @@ server.listen(0, common.mustCall(function() {
  sock.write('hello1');
  sock.write('hello2');
  sock.write('hello3\n');
-  sock.end('THUNDERMUSCLE!');
+  assert.strictEqual(sock.end('THUNDERMUSCLE!'), sock);


since you're in here making changes already, would you mind changing the string that is written to something more innocuous here?

evanlucas · 2017-06-07T13:31:58Z

Maybe also update the documentation to include the return value?

PR-URL: nodejs#13481 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net>

PR-URL: #13481 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net>

gibfahn · 2018-01-15T21:49:26Z

Release team were +1 on landing this in 6.x

PR-URL: #13481 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

PR-URL: #13481 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

PR-URL: #13481 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 5, 2017

cjihrig reviewed Jun 5, 2017

View reviewed changes

cjihrig approved these changes Jun 5, 2017

View reviewed changes

sam-github force-pushed the socket-end-return-this branch from 5ad4c92 to f6f1563 Compare June 5, 2017 19:58

lpinca approved these changes Jun 5, 2017

View reviewed changes

jasnell reviewed Jun 6, 2017

View reviewed changes

refack approved these changes Jun 6, 2017

View reviewed changes

evanlucas approved these changes Jun 7, 2017

View reviewed changes

sam-github force-pushed the socket-end-return-this branch from f6f1563 to 32d3ead Compare June 7, 2017 18:08

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 7, 2017

sam-github force-pushed the socket-end-return-this branch from 32d3ead to bb8300d Compare June 7, 2017 19:53

sam-github force-pushed the socket-end-return-this branch from bb8300d to 324f111 Compare June 7, 2017 19:54

sam-github merged commit 324f111 into nodejs:master Jun 7, 2017

sam-github deleted the socket-end-return-this branch June 7, 2017 20:04

gibfahn added the lts-watch-v6.x label Jan 15, 2018

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: return this from net.Socket.end() #13481

lib: return this from net.Socket.end() #13481

sam-github commented Jun 5, 2017

cjihrig Jun 5, 2017

cjihrig left a comment

mscdex commented Jun 5, 2017

sam-github commented Jun 5, 2017

jasnell Jun 6, 2017

evanlucas commented Jun 7, 2017

gibfahn commented Jan 15, 2018

lib: return this from net.Socket.end() #13481

lib: return this from net.Socket.end() #13481

Conversation

sam-github commented Jun 5, 2017

Checklist

Affected core subsystem(s)

cjihrig Jun 5, 2017

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

mscdex commented Jun 5, 2017

sam-github commented Jun 5, 2017

jasnell Jun 6, 2017

Choose a reason for hiding this comment

evanlucas commented Jun 7, 2017

gibfahn commented Jan 15, 2018