net: introduce `Socket#connecting` property by indutny · Pull Request #6404 · nodejs/node · GitHub

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.

cc @nodejs/collaborators @bnoordhuis

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.

LGTM with a question

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

Ack.

strictEqual would be better I guess.

Ack.

All fixed. Thanks!

LGTM

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>

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

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

👍 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>

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>

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.

@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

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

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

               three properties, e.g.
               `{ port: 12346, family: 'IPv4', address: '127.0.0.1' }`
+              ### socket.connecting

net: introduce `Socket#connecting` property by indutny · Pull Request #6404 · nodejs/node · GitHub

Uh oh!

Uh oh!

net: introduce Socket#connecting property - #6404

net: introduce Socket#connecting property#6404indutny wants to merge 3 commits intonodejs:masternodejs/node:masterfrom indutny:feature/connectingindutny/io.js:feature/connectingCopy head branch name to clipboard

Conversation

indutny commented Apr 26, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Apr 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Apr 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Apr 27, 2016

Uh oh!

thefourtheye commented Apr 27, 2016

Uh oh!

indutny commented Apr 27, 2016

Uh oh!

Fishrock123 commented Apr 27, 2016

Uh oh!

MylesBorins commented Jun 1, 2016

Uh oh!

indutny commented Jun 1, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

rvagg commented Jul 12, 2016

Uh oh!

MylesBorins commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

net: introduce Socket#connecting property#6404
indutny wants to merge 3 commits into
nodejs:masterfrom
indutny:feature/connecting

MylesBorins commented Jul 12, 2016 •
edited
Loading