Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: defines all the fields in the constructor #9116

Closed
wants to merge 1 commit into from

Conversation

vitkarpov
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 16, 2016
@@ -197,7 +197,9 @@ function ClientRequest(options, cb) {
self = null;
});

this._ended = false;
self._ended = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this._ended = false;
self._ended = false;
self.res = null;
self.aborted = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -197,7 +197,9 @@ function ClientRequest(options, cb) {
self = null;
});

this._ended = false;
self._ended = false;
self.res = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vitkarpov
Copy link
Contributor Author

vitkarpov commented Oct 16, 2016

@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

@mscdex
Copy link
Contributor

mscdex commented Oct 16, 2016

@vitkarpov
Copy link
Contributor Author

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

@mscdex
Copy link
Contributor

mscdex commented Oct 17, 2016

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

@vitkarpov
Copy link
Contributor Author

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

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

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

@cjihrig
Copy link
Contributor

cjihrig commented Oct 25, 2016

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

@vitkarpov
Copy link
Contributor Author

@cjihrig rebased

@mscdex
Copy link
Contributor

mscdex commented Oct 28, 2016

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

cjihrig commented Jan 5, 2017

@mscdex
Copy link
Contributor

mscdex commented Jan 6, 2017

@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;

@vitkarpov
Copy link
Contributor Author

@mscdex Done.

@mscdex
Copy link
Contributor

mscdex commented Jan 6, 2017

@mscdex
Copy link
Contributor

mscdex commented Jan 6, 2017

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

LGTM.

/cc @nodejs/collaborators

@targos
Copy link
Member

targos commented Jan 6, 2017

I think there are additional properties to add:

  • this.parser = null
  • this.maxHeadersCount = undefined

@MylesBorins
Copy link
Contributor

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

darai0512 added a commit to darai0512/node that referenced this pull request Apr 30, 2018
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
addaleax pushed a commit that referenced this pull request May 5, 2018
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>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
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>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
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>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants