net: introduce `Socket#connecting` property #6404

indutny · 2016-04-26T20:32:50Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

net

Description of change

There is no official way to figure out if the socket that you have on
hand is still connecting to the remote host. Introduce
Socket#connecting, which is essentially an unprefixed _connecting
property that we already had.

There is no official way to figure out if the socket that you have on hand is still connecting to the remote host. Introduce `Socket#connecting`, which is essentially an unprefixed `_connecting` property that we already had.

indutny · 2016-04-26T20:32:58Z

cc @nodejs/collaborators @bnoordhuis

imran-iq · 2016-04-26T22:04:25Z

test/parallel/test-net-connect-buffer.js

@@ -40,7 +40,7 @@ tcp.listen(common.PORT, function() {
    connectHappened = true;
  });

-  console.log('_connecting = ' + socket._connecting);
+  console.log('_connecting = ' + socket.connecting);


Should the text also be changed to 'connecting = '?

Doesn't really matter, IMO

Changing the text might save someone a cycle or two if the test fails and they start poking around trying to figure out why... probably slightly more time than it took me to write this (almost none, but non-zero).

@rmg I disagree, but will change it. Ack.

evanlucas · 2016-04-26T23:21:54Z

LGTM with a question

evanlucas · 2016-04-26T23:22:54Z

doc/api/net.md

@@ -333,6 +333,12 @@ socket as reported by the operating system. Returns an object with
 three properties, e.g.
 `{ port: 12346, family: 'IPv4', address: '127.0.0.1' }`

+### socket.connecting


Should be below socket.bufferSize to keep them in alphabetical order.

thefourtheye · 2016-04-27T02:42:09Z

test/parallel/test-net-socket-connecting.js

+  server.close();
+}).listen(common.PORT, () => {
+  const client = net.connect(common.PORT, () => {
+    assert.equal(client.connecting, false);


strictEqual would be better I guess.

indutny · 2016-04-27T02:53:41Z

All fixed. Thanks!

thefourtheye · 2016-04-27T04:01:17Z

LGTM

indutny · 2016-04-27T04:41:45Z

Landed in cee4c25, thank you!

There is no official way to figure out if the socket that you have on hand is still connecting to the remote host. Introduce `Socket#connecting`, which is essentially an unprefixed `_connecting` property that we already had. PR-URL: #6404 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Fishrock123 · 2016-04-27T13:52:14Z

We're going to need to keep _connecting anyways...

There is no official way to figure out if the socket that you have on hand is still connecting to the remote host. Introduce `Socket#connecting`, which is essentially an unprefixed `_connecting` property that we already had. PR-URL: #6404 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

* assert: `deep{Strict}Equal()` now works correctly with circular references. (Rich Trott) #6432 * debugger: Arrays are now formatted correctly in the debugger repl. (cjihrig) #6448 * deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu) #6550 - Please see our blog post for more info on the security contents of this release: - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/ * net: Introduced a `Socket#connecting` property. (Fedor Indutny) #6404 - Previously this information was only available as the undocumented, internal `_connecting` property. * process: Introduced `process.cpuUsage()`. (Patrick Mueller) #6157 * stream: `Writable#setDefaultEncoding()` now returns `this`. (Alexander Makarenko) #5040 * util: Two new additions to `util.inspect()`: - Added a `maxArrayLength` option to truncate the formatting of Arrays. (James M Snell) #6334 - This is set to `100` by default. - Added a `showProxy` option for formatting proxy intercepting handlers. (James M Snell) #6465 - Inspecting proxies is non-trivial and as such this is off by default. PR-URL: #6557

MylesBorins · 2016-06-01T22:27:40Z

@nodejs/lts @nodejs/ctc do we want to backport this in v4.5.0?

indutny · 2016-06-01T22:29:30Z

👍 from me.

There is no official way to figure out if the socket that you have on hand is still connecting to the remote host. Introduce `Socket#connecting`, which is essentially an unprefixed `_connecting` property that we already had. PR-URL: #6404 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins · 2016-07-11T20:01:09Z

I've gone ahead and landed this on v4.x-staging. it required some manual work so any eyes on it would be nice 0eef098.

If anyone has any trepidation regarding this change please let me know and it can be backed out.

There is no official way to figure out if the socket that you have on hand is still connecting to the remote host. Introduce `Socket#connecting`, which is essentially an unprefixed `_connecting` property that we already had. PR-URL: #6404 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

rvagg · 2016-07-12T05:11:58Z

I'm not so keen on this in LTS, it's not a vital and _connecting if anyone thinks it is. It's hard to justify a semver-minor bump for something like this in LTS that's not critical in any way.

This is how we currently have it framed in the LTS plan:

new features (semver-minor) may only be landed with consent of the CTC and the LTS Working

Not that I really want to have to get the CTC involved in something like this but the spirit of the LTS plan is that semver-minor changes would be minimal and should therefore be restricted to stability and security things or changes that have a very clear reason for needing to be across as many active versions of Node as possible (likley already covered by "stability and security" things). I don't see this being in that category.

MylesBorins · 2016-07-12T05:34:42Z

@rvagg that is reasonable. I've gone ahead and backed it out.

I'm going to open an issue that will outline all semver-minor commits are currently in staging so we can audit them.

net: introduce Socket#connecting property

9db9528

There is no official way to figure out if the socket that you have on hand is still connecting to the remote host. Introduce `Socket#connecting`, which is essentially an unprefixed `_connecting` property that we already had.

imran-iq reviewed Apr 26, 2016
View reviewed changes

evanlucas reviewed Apr 26, 2016
View reviewed changes

addaleax added the net Issues and PRs related to the net subsystem. label Apr 27, 2016

indutny added semver-minor PRs that contain new features and should be released in the next minor version. lts-watch-v4.x labels Apr 27, 2016

...

f906cb3

thefourtheye reviewed Apr 27, 2016
View reviewed changes

fix

1ed27a3

indutny closed this Apr 27, 2016

indutny deleted the feature/connecting branch April 27, 2016 04:41

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

MylesBorins mentioned this pull request May 16, 2016

Release Schedule for v4.4.5 and v4.5.0 nodejs/Release#102

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins added dont-land-on-v4.x and removed land-on-v4.x labels Jul 12, 2016

refack mentioned this pull request Apr 6, 2017

benchmark: readStart only after completion #12258

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: introduce `Socket#connecting` property #6404

net: introduce `Socket#connecting` property #6404

indutny commented Apr 26, 2016

indutny commented Apr 26, 2016

imran-iq Apr 26, 2016

indutny Apr 26, 2016

rmg Apr 26, 2016

indutny Apr 27, 2016

evanlucas commented Apr 26, 2016

evanlucas Apr 26, 2016

indutny Apr 27, 2016

thefourtheye Apr 27, 2016

indutny Apr 27, 2016

indutny commented Apr 27, 2016

thefourtheye commented Apr 27, 2016

indutny commented Apr 27, 2016

Fishrock123 commented Apr 27, 2016

MylesBorins commented Jun 1, 2016

indutny commented Jun 1, 2016

MylesBorins commented Jul 11, 2016

rvagg commented Jul 12, 2016

MylesBorins commented Jul 12, 2016 •

edited

Loading

net: introduce Socket#connecting property #6404

net: introduce Socket#connecting property #6404

Conversation

indutny commented Apr 26, 2016

Checklist

Affected core subsystem(s)

Description of change

indutny commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 27, 2016

thefourtheye commented Apr 27, 2016

indutny commented Apr 27, 2016

Fishrock123 commented Apr 27, 2016

MylesBorins commented Jun 1, 2016

indutny commented Jun 1, 2016

MylesBorins commented Jul 11, 2016

rvagg commented Jul 12, 2016

MylesBorins commented Jul 12, 2016 • edited Loading

net: introduce `Socket#connecting` property #6404

net: introduce `Socket#connecting` property #6404

MylesBorins commented Jul 12, 2016 •

edited

Loading