https: add extra options to Agent#getName() by princjef · Pull Request #16402 · nodejs/node · GitHub

Adds the remaining options from tls.createSecureContext() to the
string generated by Agent#getName(). This allows https.request()
to accept the options and generate unique sockets appropriately.

Checklist

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

Affected core subsystem(s)

https

See #16196 for more context/discussion

Just skimmed over it but this seems sensible to me. Thanks for the PR!

Hm, this is kind of starting to look like working with a list of options might be an easier choice…

Agreed. How about something like:

const components = [
  options.ca,
  options.cert,
  options.ciphers,
  options.key,
  options.pfx,
  options.rejectUnauthorized,
  options.servername !== options.host ? options.servername : undefined,
  options.secureProtocol,
  options.crl,
  options.honorCipherOrder,
  options.ecdhCurve,
  options.dhparam,
  options.secureOptions,
  options.sessionIdContext
];

for (const component of components) {
  name += ':';
  if (component != null) {
    name += component;
  }
}

// or:
// name += components
//   .map(component => component != null ? `:${component}` : ':')
//   .join('');

There's a potential slight difference in serialization (nulls wouldn't be added for rejectUnauthorized, honorCipherOrder, or secureOptions any more), but it seems like null would either have no impact or be invalid for all three so it feels alright to me.

Thoughts?

but it seems like null would either have no impact or be invalid for all three so it feels alright to me.

I agree.

updated

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

Updated to integrate the clientCertEngine changes from 6ee985f

Small comments, otherwise LGTM

It is better to be explicit and write component !== null && component !== undefined as that is faster than the implicit way. Using a plain for loop should also be a tad faster than the for of loop.

x != null should in fact be faster than two strict checks but yes, for/of is probably a bit slower than a plain for loop.

What's more, creating a throwaway array like that has a lot of overhead compared to the code it replaces. I expect you'll see a dip in the benchmarks.

x != null was faster with Crankshaft. That is not true anymore since Turbofan.

And yes, the array is indeed quite a overhead. I would also rather prevent that but it makes the code here much more readable.

This method is probably enough of a hot path that performance trumps readability. Unless the benchmarks show otherwise, of course - to measure is to know (which doesn't have quite the same ring to it as "meten is weten" in Dutch, but I digress.)

x != null was faster with Crankshaft. That is not true anymore since Turbofan.

It's a wash with TF (or at least it should be, the CSA code paths are practically identical) but x != null is faster with Ignition because it's just one dispatch from the interpreter loop instead of two (or three, if you include the &&.)

Probably still a wash in the grand scheme of things. If nothing else, x != null is more succinct than the long-hand x !== null && x !== undefined.

It sounds like collecting values into a temporary array may not be worth the performance hit, in which case the loop goes away anyway. If I remember correctly, the form with separate ifs doesn't have any x != null checks either (it was added to the loop form to better account for numbers/booleans than a straight truthy/falsy check), so that point becomes moot as well. So, shall I flip it back to separate if checks or keep it as an array @BridgeAR @bnoordhuis?

Don't have anything to add of value but re: the performance discussion, there's one notable case where != null is significantly slower and that's when dealing with Objects (due to the possibility of document.all).

The entries were originally sorted by alphabet. Any reason why you changed that?

I don't remember exactly but I believe it was to align with the list in tls.createSecureContext(). That being said, alphabetic probably makes more sense.

I think it would be good to keep this reference. There are other places where the agent is mentioned but not referenced. It would probably be good to make all occurances of the Agent a reference instead of removing this.

@princjef would you be so kind and rebase and address the comment by removing the loop? :-)

Ping @princjef

Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately.

Sorry for the delay @BridgeAR. The loop has been removed, the names in the docs alphabetized and the Agent references added.

Let me know if there's anything I missed

LGTM

New CI https://ci.nodejs.org/job/node-test-pull-request/12987/

LGTM

Landed in 6007a9c 🎉

Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. PR-URL: nodejs#16402 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. PR-URL: #16402 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. PR-URL: nodejs#16402 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

is this something we would want to backport to v8.x?

nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Oct 23, 2017

addaleax reviewed Oct 23, 2017

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 23, 2017

lpinca approved these changes Nov 2, 2017

View reviewed changes

princjef force-pushed the https-additional-options branch from c881bce to 100d403 Compare November 4, 2017 21:09

princjef force-pushed the https-additional-options branch from 100d403 to 7c0ba8c Compare November 13, 2017 05:19

BridgeAR approved these changes Nov 23, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

https: add extra options to Agent#getName()

44475cb

Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately.

princjef force-pushed the https-additional-options branch from 7c0ba8c to 44475cb Compare January 31, 2018 04:14

BridgeAR approved these changes Feb 7, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 7, 2018

BridgeAR requested review from apapirovski, jasnell and mcollina February 7, 2018 13:33

mcollina approved these changes Feb 7, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

BridgeAR closed this Feb 10, 2018

BridgeAR mentioned this pull request Feb 10, 2018

crypto: Expose the public key of a certificate & cert sha256 #17690

Closed

4 tasks

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

lpinca mentioned this pull request Apr 7, 2019

lib: refactor tls options handle in https.js #27118

Closed

2 tasks

https: add extra options to Agent#getName() by princjef · Pull Request #16402 · nodejs/node · GitHub

Uh oh!

Uh oh!

https: add extra options to Agent#getName() - #16402

https: add extra options to Agent#getName()#16402princjef wants to merge 1 commit intonodejs:masternodejs/node:masterfrom princjef:https-additional-optionsprincjef/node:https-additional-optionsCopy head branch name to clipboard

Conversation

princjef commented Oct 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

princjef commented Oct 23, 2017

Uh oh!

addaleax left a comment

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!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Nov 2, 2017

Uh oh!

princjef commented Nov 13, 2017

Uh oh!

BridgeAR left a comment

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!

BridgeAR Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

princjef Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 19, 2018

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

princjef commented Jan 31, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 7, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 10, 2018

Uh oh!

MylesBorins commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

https: add extra options to Agent#getName()#16402
princjef wants to merge 1 commit into
nodejs:masterfrom
princjef:https-additional-options

BridgeAR Nov 23, 2017 •
edited
Loading

princjef Nov 26, 2017 •
edited
Loading