https: make opts optional & immutable when create #13599

XadillaX · 2017-06-10T18:14:46Z

opts in createServer will be immutable that won't change origional
opts value. What's more, it's optional which can make requestListener
be the first argument.

Refs: #13584

Checklist

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

Affected core subsystem(s)

https

`opts` in `createServer` will be immutable that won't change origional opts value. What's more, it's optional which can make `requestListener` be the first argument. Refs: nodejs#13584

mscdex · 2017-06-10T18:19:25Z

doc/api/https.md

@@ -46,7 +46,7 @@ added: v8.0.0

 See [`http.Server#keepAliveTimeout`][].

-## https.createServer(options[, requestListener])
+## https.createServer([options][, requestListener])


I don't think we have to change this in the documentation, since tls.Server() will still require a configuration (a certificate and key at minimum). The benefit of guarding early before tls.Server() is called is to avoid strange JavaScript runtime errors. Huh... I take that back, it doesn't seem to enforce a minimum configuration...

But a test in either way should be added.

@refack the test is in test/parallel/test-https-immutable-options.js, which tested in server2.

Need a test like

// validate that `createServer` can work with no arguments const server2 = https.createServer(); assert.ok(server2); assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);

@refack done.

refack

1 style nit
1 request for test (assert signature)

refack · 2017-06-10T18:23:04Z

lib/https.js

+    requestListener = opts;
+    opts = undefined;
+  }
+  opts = opts ? util._extend({}, opts) : {};


I think you can use Object.assign (and give opts a default in L34)

@refack Did you benchmark that util._extend is faster than Object.assign by now? Last I heard it was not, which is why we still use util._extend for most internals.

@addaleax I was about to say that but I'm not sure it matters in the case of starting a server. *shrug*

util._extend() does have significantly more usage in core though according to a quick grep.

Hmmmm, so I'm waiting for a result.

IMHO this is a "cold spot" so I opt for standard 🤷‍♂️
Any way this could be simply

opts = util._extend({}, opts);

@XadillaX I think you're free to decide

> console.time('assign');for(var i=0; i < 1E7; ++i) { Object.assign({1:1, 2:2, 3:3}, {a:1, b:2, c:3}); };console.timeEnd('assign') assign: 9825.217ms undefined > console.time('_extend');for(var i=0; i < 1E7; ++i) { u._extend({1:1, 2:2, 3:3}, {a:1, b:2, c:3}); };console.timeEnd('_extend') _extend: 6174.819ms undefined

so util._extend is ~ 60% of Object.assign.
IMHO if it's not better than 50%, Object.assign should be prefered

If util._extend uses more than Object.assign, I think I can change it back to util._extend.

@refack, And if necessary, I think we can open a new PR about changing all util._extend to Object.assign in this file next time. Because it not only contains one util._extend here.

@refack, And if necessary, I think we can open a new PR about changing all util._extend to Object.assign in this file next time. Because it not only contains one util._extend here.

We can do this when V8 optimizes Object.assign to be <= util._extend...

refack · 2017-06-10T18:36:33Z

doc/api/https.md

@@ -46,7 +46,7 @@ added: v8.0.0

 See [`http.Server#keepAliveTimeout`][].

-## https.createServer(options[, requestListener])
+## https.createServer([options][, requestListener])


But a test in either way should be added.

mscdex · 2017-06-10T18:44:29Z

test/parallel/test-https-immutable-options.js

+assert.deepStrictEqual(opts, { foo: 'bar' });
+assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);
+
+const mustNotCall = common.mustNotCall('dummy callback');


It might be best to just omit the argument here, since if the function is called for some odd reason the default message should be slightly more descriptive and good enough.

mscdex · 2017-06-10T18:47:59Z

test/parallel/test-https-immutable-options.js

+const server1 = https.createServer(opts);
+
+assert.deepStrictEqual(opts, { foo: 'bar' });
+assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);


I think it should suffice to just compare the NPNProtocols references. That way we know for sure that our options weren't overridden:

assert.strictEqual(server1.NPNProtocols, dftProtocol.NPNProtocols);

@mscdex I think we can't, because NPNProtocols hasn't been cached yet. 2 calls to tls.convertNPNProtocols won't return the same reference.

Ah ok I see it now. Perhaps we should at least choose a different set of protocols then, so as to differentiate from the default NPN protocols (perhaps just use one protocol?).

refack · 2017-06-10T18:49:23Z

doc/api/https.md

@@ -46,7 +46,7 @@ added: v8.0.0

 See [`http.Server#keepAliveTimeout`][].

-## https.createServer(options[, requestListener])
+## https.createServer([options][, requestListener])


Need a test like

// validate that `createServer` can work with no arguments const server2 = https.createServer(); assert.ok(server2); assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);

refack · 2017-06-10T18:51:21Z

lib/https.js

+    requestListener = opts;
+    opts = undefined;
+  }
+  opts = opts ? Object.assign({}, opts) : {};


I think you can change this to

opts = Object.assign({}, opts);

lpinca · 2017-06-10T19:26:08Z

test/parallel/test-https-immutable-options.js

+const server2 = https.createServer(mustNotCall);
+
+assert.strictEqual(server2.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);
+assert.strictEqual(mustNotCall, server2._events.request);


Can we replace this with

assert.strictEqual(server2.listeners('request'), 1); assert.strictEqual(server2.listeners('request')[0], mustNotCall);

mscdex · 2017-06-10T19:38:07Z

test/parallel/test-https-immutable-options.js

+assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);
+
+const mustNotCall = common.mustNotCall();
+const server2 = https.createServer(mustNotCall);


The comment below doesn't match the behavior of this line. If we're testing no arguments, it should just be https.createServer().

Otherwise we could add a separate test below for no arguments.

lpinca · 2017-06-10T19:44:30Z

test/parallel/test-https-immutable-options.js

+
+// validate that `createServer` can work with no arguments
+tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol);
+assert.ok(server2);


Nit: It doesn't harm but I think this is redundant. An error would be thrown below if server2 is falsy.

@refack, Shall I?

~~It's a style we now use more often (helps frame every test case), but IMHO it's up to you...~~ that was for the other comment
I'm 👍 for keeping it, but maybe put if just below the const server2 =

lpinca · 2017-06-10T20:01:38Z

test/parallel/test-https-argument-of-creating.js

+
+// test for immutable `opts`
+const opts = { foo: 'bar', NPNProtocols: [ 'http/1.1' ] };
+const server1 = https.createServer(opts);


Tiny nit: instead of using server1, server2, and server3 we can use block scope.

{ // test for immutable `opts` } { // validate that `createServer` can work with the only argument requestListener }

It's a style we now use more often (helps frame every test case), but IMHO it's up to you...

mscdex · 2017-06-10T20:21:11Z

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

refack

💯
LGTM as is

mscdex · 2017-06-10T21:52:55Z

LGTM

cjihrig

LGTM with a comment

cjihrig · 2017-06-11T12:35:06Z

test/parallel/test-https-argument-of-creating.js

+'use strict';
+
+const common = require('../common');
+


Can you add this:

if (!common.hasCrypto) { common.skip('missing crypto'); return; }

yorkie

LGTM with nit

yorkie · 2017-06-12T02:31:39Z

lib/https.js

  if (!(this instanceof Server)) return new Server(opts, requestListener);

+  if (typeof opts === 'function') {
+    requestListener = opts;
+    opts = undefined;


Minor nit: The optional value is an object, but here reset to undefined, this does not appear consistent :)

@yorkie, Because I think util._extend({}, undefined) performance is better than util._extend({}, {})

Then I think you would make the opts default be unset as well for performance. BTW perf is not that crucial here than consistence and readability, this constructor needn't be called too much times in any of applications :)

The problem is opts = util._extend({}, opts); shows it's not a consistent.

Do you mean that?

... const realOpts = util._extend({}, opts);

The mean problem is the default value, removing that like https://github.com/nodejs/node/pull/13599/files#r121620077 works for me :)

jasnell · 2017-06-12T15:04:42Z

lib/https.js

@@ -31,9 +31,15 @@ const inherits = util.inherits;
 const debug = util.debuglog('https');
 const { urlToOptions, searchParamsSymbol } = require('internal/url');

-function Server(opts, requestListener) {
+function Server(opts = {}, requestListener) {


Note that using default values is potentially a breaking change given that it changes the value of Server.length

Shall I delete the default value?

refack · 2017-06-12T18:46:33Z

CITGM to look for breakage: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/871/
CI: https://ci.nodejs.org/job/node-test-commit/10517/

refack

LGTM
if test still pass removing default was a good call

refack · 2017-06-14T13:27:25Z

Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/8648/

`opts` in `createServer` will be immutable that won't change origional opts value. What's more, it's optional which can make `requestListener` be the first argument. PR-URL: nodejs#13599 Fixes: nodejs#13584 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

refack · 2017-06-14T19:16:58Z

Landed in c1c2267

`opts` in `createServer` will be immutable that won't change origional opts value. What's more, it's optional which can make `requestListener` be the first argument. PR-URL: #13599 Fixes: #13584 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

gibfahn · 2018-01-15T21:52:28Z

Release team decided not to land on v6.x, if you disagree let us know.

https: make opts optional & immutable when create

26d4efb

`opts` in `createServer` will be immutable that won't change origional opts value. What's more, it's optional which can make `requestListener` be the first argument. Refs: nodejs#13584

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

mscdex reviewed Jun 10, 2017

View reviewed changes

refack suggested changes Jun 10, 2017

View reviewed changes

XadillaX force-pushed the feature/https-optional-immutable-opt branch from 35b1e77 to 26d4efb Compare June 10, 2017 18:39

mscdex reviewed Jun 10, 2017

View reviewed changes

use Object.assign to instead of util._extend

2c7ff7b

mscdex reviewed Jun 10, 2017

View reviewed changes

use the default message in common.mustNotCall

b3a1ac6

refack suggested changes Jun 10, 2017

View reviewed changes

refack reviewed Jun 10, 2017

View reviewed changes

lpinca reviewed Jun 10, 2017

View reviewed changes

update after reviewing

9414765

refack approved these changes Jun 10, 2017

View reviewed changes

mscdex reviewed Jun 10, 2017

View reviewed changes

lpinca reviewed Jun 10, 2017

View reviewed changes

add test case for no argument

c798af8

lpinca reviewed Jun 10, 2017

View reviewed changes

use blocks to instead of server1, server2, ...

5de8cf2

lpinca approved these changes Jun 10, 2017

View reviewed changes

refack approved these changes Jun 10, 2017

View reviewed changes

cjihrig approved these changes Jun 11, 2017

View reviewed changes

add test skip

dc2291c

yorkie approved these changes Jun 12, 2017

View reviewed changes

jasnell reviewed Jun 12, 2017

View reviewed changes

delete default param to prevent some breaking changes

2232fad

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 13, 2017

refack approved these changes Jun 13, 2017

View reviewed changes

refack self-assigned this Jun 14, 2017

jasnell approved these changes Jun 14, 2017

View reviewed changes

refack closed this Jun 14, 2017

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

refack removed their assignment Oct 12, 2018

LiviaMedeiros mentioned this pull request May 27, 2022

test: add common.mustNotMutateObjectDeep() #43196

Merged

https: make opts optional & immutable when create #13599

https: make opts optional & immutable when create #13599

Conversation

XadillaX commented Jun 10, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex Jun 10, 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

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 10, 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

XadillaX Jun 10, 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

mscdex Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 10, 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

mscdex Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jun 10, 2017

refack left a comment

Choose a reason for hiding this comment

mscdex commented Jun 10, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorkie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

yorkie Jun 13, 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

refack commented Jun 12, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

refack commented Jun 14, 2017

refack commented Jun 14, 2017

gibfahn commented Jan 15, 2018

XadillaX commented Jun 10, 2017 •

edited

Loading

mscdex Jun 10, 2017 •

edited

Loading

mscdex Jun 10, 2017 •

edited

Loading

XadillaX Jun 10, 2017 •

edited

Loading

mscdex Jun 10, 2017 •

edited

Loading

mscdex Jun 10, 2017 •

edited

Loading

mscdex Jun 10, 2017 •

edited

Loading

refack Jun 10, 2017 •

edited

Loading

XadillaX Jun 12, 2017 •

edited

Loading

yorkie Jun 13, 2017 •

edited

Loading

refack commented Jun 12, 2017 •

edited

Loading