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

princjef · 2017-10-23T06:45:56Z

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

princjef · 2017-10-23T06:46:49Z

See #16196 for more context/discussion

addaleax

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

addaleax · 2017-10-23T07:10:03Z

lib/https.js

+
+  name += ':';
+  if (options.sessionIdContext)
+    name += options.sessionIdContext;


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.

lpinca · 2017-11-02T07:43:37Z

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

princjef · 2017-11-13T05:24:24Z

Updated to integrate the clientCertEngine changes from 6ee985f

BridgeAR

Small comments, otherwise LGTM

BridgeAR · 2017-11-23T10:45:14Z

lib/https.js

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


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

BridgeAR · 2017-11-23T10:45:48Z

doc/api/https.md

+The following additional `options` from [`tls.connect()`][] are also accepted:
+`pfx`, `key`, `passphrase`, `cert`, `ca`, `ciphers`, `clientCertEngine`, `crl`,
+`dhparam`, `ecdhCurve`, `honorCipherOrder`,`rejectUnauthorized`, `secureOptions`,
+`secureProtocol`, `servername`, `sessionIdContext`


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.

BridgeAR · 2017-11-23T10:50:30Z

doc/api/https.md

@@ -251,7 +252,6 @@ const req = https.request(options, (res) => {
 });
 ```

-[`Agent`]: #https_class_https_agent


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.

BridgeAR · 2018-01-19T16:18:26Z

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

BridgeAR · 2018-01-31T01:51:47Z

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.

princjef · 2018-01-31T04:17:59Z

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

BridgeAR

LGTM

BridgeAR · 2018-02-07T03:02:09Z

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

mcollina

LGTM

BridgeAR · 2018-02-10T13:17:52Z

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

MylesBorins · 2018-08-07T18:55:01Z

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 pending requests for changes, and a CI started. label Feb 7, 2018

BridgeAR requested review from apapirovski, mcollina and jasnell 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

princjef commented Oct 23, 2017

princjef commented Oct 23, 2017

addaleax left a comment

addaleax Oct 23, 2017

princjef Oct 23, 2017

lpinca Nov 2, 2017

princjef Nov 4, 2017

lpinca commented Nov 2, 2017

princjef commented Nov 13, 2017

BridgeAR left a comment

BridgeAR Nov 23, 2017

bnoordhuis Nov 23, 2017

BridgeAR Nov 23, 2017 •

edited

Loading

bnoordhuis Nov 23, 2017

princjef Nov 26, 2017 •

edited

Loading

apapirovski Nov 26, 2017

BridgeAR Nov 23, 2017

princjef Nov 26, 2017

BridgeAR Nov 23, 2017

BridgeAR commented Jan 19, 2018

BridgeAR commented Jan 31, 2018

princjef commented Jan 31, 2018

BridgeAR left a comment

BridgeAR commented Feb 7, 2018

mcollina left a comment

BridgeAR commented Feb 10, 2018

MylesBorins commented Aug 7, 2018

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

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

Conversation

princjef commented Oct 23, 2017

Checklist

Affected core subsystem(s)

princjef commented Oct 23, 2017

addaleax left a comment

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

Choose a reason for hiding this comment

lpinca commented Nov 2, 2017

princjef commented Nov 13, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

princjef Nov 26, 2017 • edited Loading

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

Choose a reason for hiding this comment

BridgeAR commented Jan 19, 2018

BridgeAR commented Jan 31, 2018

princjef commented Jan 31, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 7, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 10, 2018

MylesBorins commented Aug 7, 2018

BridgeAR Nov 23, 2017 •

edited

Loading

princjef Nov 26, 2017 •

edited

Loading