http: add doc deprecation for private props #10941

mscdex · 2017-01-21T17:31:38Z

This is one proposed solution to the current backwards compatibility issue (see #10558) in master with end users who directly access outgoingMessage._headers/outgoingMessage._headerNames.

This PR will be simplified some once #10805 is landed (which should land before this PR). Specifically, the ._headers getter can simply just return this.getHeaders().

/cc @nodejs/collaborators

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

Checklist

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

Affected core subsystem(s)

http

targos · 2017-01-21T17:34:25Z

lib/_http_client.js

@@ -182,7 +182,7 @@ function ClientRequest(options, cb) {
                      'client');
    }
    self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n',
-                      self._headers);
+                      self._headersStore);


Maybe use a Symbol to hide it more?

Does accessing a value via a Symbol have a perf hit vs. normal property access in master?

I don't know. I'll look into making a benchmark.

Results with the following benchmark (is it worth adding to the repo?): targos@d50c790

# master $ ./node benchmark/es/object-property-bench.js es/object-property-bench.js millions=1000 method="property": 1,137.5161172810194 es/object-property-bench.js millions=1000 method="string": 1,133.78613592118 es/object-property-bench.js millions=1000 method="variable": 1,137.9577172760735 es/object-property-bench.js millions=1000 method="symbol": 1,133.090467628974 # v7.4.0 $ node benchmark/es/object-property-bench.js es/object-property-bench.js millions=1000 method="property": 1,136.7741112855451 es/object-property-bench.js millions=1000 method="string": 1,136.7865674716409 es/object-property-bench.js millions=1000 method="variable": 1,135.1437992275612 es/object-property-bench.js millions=1000 method="symbol": 1,136.4064052769731 # v4.7.1 $ node benchmark/es/object-property-bench.js es/object-property-bench.js millions=1000 method="property": 1,135.4850535131566 es/object-property-bench.js millions=1000 method="string": 1,134.4381133016345 es/object-property-bench.js millions=1000 method="variable": 78.62998231306928 es/object-property-bench.js millions=1000 method="symbol": 81.82481813228071

Ok so it might be worth sticking to properties for now.

As far as adding the benchmark, it could be useful. Although I'd probably put it in misc/ instead of es/ since it doesn't primarily deal with es6+ alternatives (which es/ seems to contain).

it might be worth sticking to properties for now

I don't think so. The numbers are very close and on multiple runs, the order varies a lot. It would be worth only for v4.x

@targos @mscdex this matches my findings. In practice I did not see any perf impact in using symbols since v7.

I've been testing this like crazy lately and from everything I have seen there is essentially zero perf impact from using symbols.

jasnell · 2017-01-23T03:15:10Z

+1 to using a symbol instead.

mscdex · 2017-01-25T21:22:18Z

@jasnell @targos Switched to symbol. CI: https://ci.nodejs.org/job/node-test-pull-request/6054/

thefourtheye · 2017-01-28T07:39:02Z

Isn't this a major change?

mscdex · 2017-02-19T21:33:42Z

This PR needs at least one more approval from a @nodejs/ctc member in order for it to land (again, after #10805 lands).

mscdex · 2017-02-20T22:35:01Z

Updated PR to use one of the new http API methods and to reuse StorageObject instead of another custom storage object constructor.

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

rvagg · 2017-02-21T02:37:33Z

do we have any ecosystem usage metrics for this? that's my only concern before agreeing.

mscdex · 2017-02-21T03:00:42Z

@rvagg I don't know, all I can do is run in CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/586/

express/send has already started the process of migrating to using the existing getHeader() instead of _headers FWIW.

rvagg · 2017-02-21T03:02:08Z

@ChALkeR are you still the one to find this data?

dougwilson · 2017-02-22T05:30:36Z

P.S. I know I shouldn't have done this, but had some code that was

function clearHeaders (res) {
  res._headers = {}
  res._headerNames = {}
}

Obviously needs to change. Right now works on Node.js 8, but this landing will break even that without any warning. Should probably keep that working with a warning if the getters would keep working, right?

mscdex · 2017-02-22T05:37:14Z

@dougwilson Yeah I'm not sure what the best solution to that would be as far as the behavior of the setter, since we'd probably have to just copy the values out of the object instead of just using the same object reference (because the internal representation changed). Should we blindly copy values in the ._headers setter or should we pass each name and value to .setHeader()?

Thoughts on this @nodejs/collaborators ?

mscdex · 2017-03-06T02:24:06Z

I'll land this on Tuesday if there are no more objections before then.

Here's another CI: https://ci.nodejs.org/job/node-test-pull-request/6720/

mikeal · 2017-03-06T21:47:49Z

It looks like a lot of reverse compatibility work has been done here, can we get a redux of what won't work anymore after this change?

evanlucas · 2017-03-07T00:26:59Z

I'm still a bit concerned with deprecating res._headers. Although it isn't documented as a public api, it is used in so many places. Is it really justifiable to break the ecosystem like this?

mscdex · 2017-03-07T01:30:35Z

@mikeal As far as I know there shouldn't really be any difference with the getters/setters in place.

@evanlucas I don't understand the concern, there is no runtime deprecation anymore. It's a documentation deprecation. Also there are now other public http APIs that cover all of the use cases I've seen thus far for accessing the private properties.

evanlucas · 2017-03-08T00:03:16Z

@mscdex even with a docs only deprecation, that still more than likely means we are going to remove it eventually. I'm just not sure whether it is justified.

I do seem to be in the minority on this one though so ¯\(ツ)/¯

mscdex · 2017-03-08T01:14:27Z

@evanlucas Well yes, that is the plan for deprecations. I don't see how it's a problem considering it's only a docs deprecation for v8.0.0 and there are public methods that cover the current use cases for these private properties (these methods should be backported to v4.x and v6.x). IMHO there will be plenty of time for people to convert their code to use these new methods before the old, private properties become a runtime deprecation and eventually removed.

This PR also fixes a backwards compatibility issue that currently exists in master, so that is why I've been wanting to get this landed sooner than later.

mscdex · 2017-03-09T08:44:41Z

A day late, but last CI before landing: https://ci.nodejs.org/job/node-test-pull-request/6759/

and last CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/633/

PR-URL: nodejs#10941 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

mscdex added http Issues or PRs related to the http subsystem. wip Issues and PRs that are still a work in progress. labels Jan 21, 2017

nodejs-github-bot added http Issues or PRs related to the http subsystem. dont-land-on-v7.x labels Jan 21, 2017

mscdex mentioned this pull request Jan 21, 2017

http: various performance improvements #10558

Merged

3 tasks

targos reviewed Jan 21, 2017

View reviewed changes

mscdex force-pushed the http-outgoingmessage-headers-deprecate branch from fd32784 to 16bd79c Compare January 25, 2017 21:20

nodejs-github-bot added the dont-land-on-v7.x label Jan 25, 2017

mscdex removed dont-land-on-v7.x wip Issues and PRs that are still a work in progress. labels Jan 25, 2017

jasnell approved these changes Jan 27, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 30, 2017

mscdex mentioned this pull request Feb 1, 2017

http: add new functions to OutgoingMessage #10805

Merged

4 tasks

addaleax added this to the 8.0.0 milestone Feb 15, 2017

mscdex mentioned this pull request Feb 19, 2017

module: improve require() performance #10789

Merged

3 tasks

ChALkeR self-requested a review February 19, 2017 09:31

mscdex force-pushed the http-outgoingmessage-headers-deprecate branch 2 times, most recently from ce2aab4 to 953b97e Compare February 20, 2017 22:32

mscdex mentioned this pull request Feb 21, 2017

this.res._headers has changed in node master koajs/koa#903

Closed

mscdex added the ctc-review label Feb 21, 2017

Trott mentioned this pull request Mar 6, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-08 nodejs/CTC#77

Closed

mscdex force-pushed the http-outgoingmessage-headers-deprecate branch from 8126753 to 0cc96d1 Compare March 7, 2017 01:33

mscdex force-pushed the http-outgoingmessage-headers-deprecate branch from 0cc96d1 to 9be372f Compare March 9, 2017 08:43

mscdex added 4 commits March 9, 2017 05:50

doc: deprecate private http properties

86996c5

PR-URL: nodejs#10941 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

http: use Symbol for outgoing headers

940b530

PR-URL: nodejs#10941 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

http: support old private properties and function

b377034

PR-URL: nodejs#10941 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

http: reuse existing StorageObject

8243ca0

PR-URL: nodejs#10941 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

mscdex force-pushed the http-outgoingmessage-headers-deprecate branch from 9be372f to 8243ca0 Compare March 9, 2017 10:50

mscdex changed the title ~~http: add deprecation warnings for private props~~ http: add doc deprecation for private props Mar 9, 2017

mscdex merged commit 8243ca0 into nodejs:master Mar 9, 2017

mscdex removed the ctc-review label Mar 9, 2017

mscdex deleted the http-outgoingmessage-headers-deprecate branch March 9, 2017 10:52

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

kevinoid mentioned this pull request May 31, 2017

matchHeader doesn't work in Node 8.x nock/nock#925

Closed

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: add doc deprecation for private props #10941

http: add doc deprecation for private props #10941

mscdex commented Jan 21, 2017 •

edited

Loading

targos Jan 21, 2017

mscdex Jan 21, 2017

targos Jan 21, 2017

targos Jan 21, 2017 •

edited

Loading

mscdex Jan 21, 2017

targos Jan 22, 2017

mcollina Jan 22, 2017

jasnell Jan 23, 2017

jasnell commented Jan 23, 2017

mscdex commented Jan 25, 2017

thefourtheye commented Jan 28, 2017

mscdex commented Feb 19, 2017

mscdex commented Feb 20, 2017

rvagg commented Feb 21, 2017

mscdex commented Feb 21, 2017 •

edited

Loading

rvagg commented Feb 21, 2017

dougwilson commented Feb 22, 2017

mscdex commented Feb 22, 2017 •

edited

Loading

mscdex commented Mar 6, 2017

mikeal commented Mar 6, 2017

evanlucas commented Mar 7, 2017

mscdex commented Mar 7, 2017

evanlucas commented Mar 8, 2017

mscdex commented Mar 8, 2017 •

edited

Loading

mscdex commented Mar 9, 2017

http: add doc deprecation for private props #10941

http: add doc deprecation for private props #10941

Conversation

mscdex commented Jan 21, 2017 • edited Loading

Checklist

Affected core subsystem(s)

targos Jan 21, 2017

Choose a reason for hiding this comment

mscdex Jan 21, 2017

Choose a reason for hiding this comment

targos Jan 21, 2017

Choose a reason for hiding this comment

targos Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex Jan 21, 2017

Choose a reason for hiding this comment

targos Jan 22, 2017

Choose a reason for hiding this comment

mcollina Jan 22, 2017

Choose a reason for hiding this comment

jasnell Jan 23, 2017

Choose a reason for hiding this comment

jasnell commented Jan 23, 2017

mscdex commented Jan 25, 2017

thefourtheye commented Jan 28, 2017

mscdex commented Feb 19, 2017

mscdex commented Feb 20, 2017

rvagg commented Feb 21, 2017

mscdex commented Feb 21, 2017 • edited Loading

rvagg commented Feb 21, 2017

dougwilson commented Feb 22, 2017

mscdex commented Feb 22, 2017 • edited Loading

mscdex commented Mar 6, 2017

mikeal commented Mar 6, 2017

evanlucas commented Mar 7, 2017

mscdex commented Mar 7, 2017

evanlucas commented Mar 8, 2017

mscdex commented Mar 8, 2017 • edited Loading

mscdex commented Mar 9, 2017

mscdex commented Jan 21, 2017 •

edited

Loading

targos Jan 21, 2017 •

edited

Loading

mscdex commented Feb 21, 2017 •

edited

Loading

mscdex commented Feb 22, 2017 •

edited

Loading

mscdex commented Mar 8, 2017 •

edited

Loading