http: defines all the fields in the constructor by vitkarpov · Pull Request #9116 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Description of change

res and abort fields are now defined in the constructor and not suddenly appear during the runtime. This makes hidden class stable and the heap snapshot more verbose.

Ref: #8912
PR-URL: #9116

This change doesn't affect anything, but I found two different ways across the single constructor: self and this. Seems it should be consistent but I don't know the agreement about it.

@bnoordhuis didn't you mention in another issue recently that we should prefer this and an arrow function over self?

If we're setting this to zero now, we should both remove ClientRequest.prototype.aborted = undefined; and change the if (this.aborted === undefined) check to if (!this.aborted) below.

Perhaps we should also change delete req.res later in this file to req.res = null?

@mscdex made some fixes after review:

removed aborted from prototype, it's unnecessary anymore
the right check for this.aborted
do not delete res from object anymore, just null it

Also, started to use this, not self in the constructor

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

Guys, why do windows tests fail? Seems it's not the result of this changes, considering it's about JavaScript code, isn't it?

@vitkarpov In this case, the Windows failure is the result of a CI infrastructure issue and is unrelated to this PR.

I'm not sure why aix failed though, it seems it couldn't find the built node binary for some reason. /cc @mhdawson ?

The ARM failure looks unrelated.

Yep, I've noticed that some other prs behave the same.

Guys, what's gonna happen with this pr? I'm ready to rebase the branch when it will be about to be merged, but I'm not sure about the process (there's not milestone tag or something).

Minimally, this needs a rebase, collaborator approval, and a new CI run.

@cjihrig rebased

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

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

@vitkarpov
A couple of things to note:

All of these assignments (including the existing this._ended = false) need to happen earlier in the constructor before any possible early returns. I would suggest placing them before these lines in the constructor:

  var called = false;
  if (self.socketPath) {
  // ...

These also need to be added to the constructor:

  this.timeoutCb = null;
  this.upgradeOrConnect = false;

@mscdex Done.

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

CI is green, except for an unrelated CI infrastructure failure.

LGTM.

/cc @nodejs/collaborators

I think there are additional properties to add:

this.parser = null
this.maxHeadersCount = undefined

It's usually helpful to add a little comment on these, as it helps in the long run (see _readableState as an example).

@mcollina I'm sorry, but I'm not sure I get it right, do you mean this comment?

I think there are additional properties to add

Hm, I greped a bit and found even more fields. Seems this needs to be processed the same way, doesn't it?

@jasnell @cjihrig Also, maybe it's a good idea to make some special setter-methods for such cases or it's overkill? (so the outer code doesn't set anything to request instance directly, because it's hard to control beyond)

@targos is right, those additional properties seem to also be used on the ClientRequest object, although I would suggest using null instead of undefined for maxHeadersCount since it makes it look more intentional.

Hm, I greped a bit and found even more fields. Seems this needs to be processed the same way, doesn't it?

@vitkarpov socket and connection are already defined in OutgoingMessage's constructor, which is what ClientRequest inherits from.

btw I used Object.seal(this) at the end of the constructor and ran the tests to find the missing properties.

I would suggest using null instead of undefined for maxHeadersCount

@mscdex maybe it should be always a number and 2000 by default in constructor? (I'm not sure does this break anything, hope tests illuminate :)

As this has not been backported I'm setting to dont-land

In Refs, http.Server's maxHeadersCount field was defined in the constructor to make hidden class stable and so on. Also in https.Server, we can use maxHeadersCount the same as http via connectionListener. So, defines it in the constructor and documentation. Refs: nodejs#9116

In Refs, http.Server's maxHeadersCount field was defined in the constructor to make hidden class stable and so on. Also in https.Server, we can use maxHeadersCount the same as http via connectionListener. So, defines it in the constructor and documentation. Refs: #9116 PR-URL: #20359 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 16, 2016

vitkarpov commented Oct 16, 2016

View reviewed changes

mscdex reviewed Oct 16, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2016

jasnell approved these changes Jan 6, 2017

View reviewed changes

cjihrig approved these changes Jan 6, 2017

View reviewed changes

mcollina reviewed Jan 6, 2017

View reviewed changes

MylesBorins added the backport-requested-v6.x label May 8, 2017

MylesBorins added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 14, 2017

darai0512 mentioned this pull request Apr 22, 2018

https: defines maxHeadersCount in the constructor darai0512/node#4

Open

4 tasks

darai0512 mentioned this pull request Apr 27, 2018

https: defines maxHeadersCount in the constructor #20359

Closed

4 tasks

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#705

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#246

Open

This was referenced Nov 30, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#826

Open

[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#846

Open

http: defines all the fields in the constructor by vitkarpov · Pull Request #9116 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: defines all the fields in the constructor - #9116

http: defines all the fields in the constructor#9116vitkarpov wants to merge 1 commit intonodejs:masternodejs/node:masterfrom vitkarpov:issue-8912Copy head branch name to clipboard

Conversation

vitkarpov commented Oct 16, 2016

Checklist

Description of change

Uh oh!

vitkarpov Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

vitkarpov commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Oct 16, 2016

Uh oh!

vitkarpov commented Oct 17, 2016

Uh oh!

mscdex commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitkarpov commented Oct 17, 2016

Uh oh!

vitkarpov commented Oct 25, 2016

Uh oh!

cjihrig commented Oct 25, 2016

Uh oh!

vitkarpov commented Oct 28, 2016

Uh oh!

mscdex commented Oct 28, 2016

Uh oh!

cjihrig commented Jan 5, 2017

Uh oh!

mscdex commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitkarpov commented Jan 6, 2017

Uh oh!

mscdex commented Jan 6, 2017

Uh oh!

mscdex commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 6, 2017

Uh oh!

mcollina Jan 6, 2017

Choose a reason for hiding this comment

Uh oh!

vitkarpov Jan 6, 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!

vitkarpov commented Jan 6, 2017

Uh oh!

mscdex commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 6, 2017

Uh oh!

vitkarpov commented Jan 6, 2017

Uh oh!

MylesBorins commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

http: defines all the fields in the constructor#9116
vitkarpov wants to merge 1 commit into
nodejs:masterfrom
vitkarpov:issue-8912

vitkarpov commented Oct 16, 2016 •
edited
Loading

mscdex commented Oct 17, 2016 •
edited
Loading

mscdex commented Jan 6, 2017 •
edited
Loading

mscdex commented Jan 6, 2017 •
edited
Loading

vitkarpov Jan 6, 2017 •
edited
Loading

mscdex commented Jan 6, 2017 •
edited
Loading

mscdex commented Jan 6, 2017 •
edited
Loading