Skip to content

Conversation

@indutny
Copy link
Member

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
Copy link
MemberAuthor

cc @nodejs/collaborators @bnoordhuis

});

console.log('_connecting = '+socket._connecting);
console.log('_connecting = '+socket.connecting);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really matter, IMO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@evanlucas
Copy link
Contributor

LGTM with a question

doc/api/net.md Outdated
three properties, e.g.
`{port: 12346, family: 'IPv4', address: '127.0.0.1'}`

### socket.connecting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@addaleaxaddaleax added the net Issues and PRs related to the net subsystem. label Apr 27, 2016
@indutnyindutny 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
server.close();
}).listen(common.PORT, () =>{
const client = net.connect(common.PORT, () =>{
assert.equal(client.connecting, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strictEqual would be better I guess.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@indutny
Copy link
MemberAuthor

All fixed. Thanks!

@thefourtheye
Copy link
Contributor

LGTM

@indutny
Copy link
MemberAuthor

Landed in cee4c25, thank you!

@indutnyindutny closed this Apr 27, 2016
@indutnyindutny deleted the feature/connecting branch April 27, 2016 04:41
indutny added a commit that referenced this pull request Apr 27, 2016
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 <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Fishrock123
Copy link
Contributor

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

Fishrock123 pushed a commit that referenced this pull request May 4, 2016
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 <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Fishrock123 added a commit that referenced this pull request May 5, 2016
* 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
Fishrock123 added a commit that referenced this pull request May 5, 2016
* 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
Fishrock123 added a commit that referenced this pull request May 5, 2016
* 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
Copy link
Contributor

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

@indutny
Copy link
MemberAuthor

👍 from me.

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
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 <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins
Copy link
Contributor

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.

MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
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 <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@rvagg
Copy link
Member

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
Copy link
Contributor

MylesBorins commented Jul 12, 2016

@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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

netIssues and PRs related to the net subsystem.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@indutny@evanlucas@thefourtheye@Fishrock123@MylesBorins@rvagg@rmg@mscdex@imran-iq@addaleax