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.

Sign up for GitHub

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

Jump to bottom

https: refactor to use http internals #16395

Closed
wants to merge 1 commit into from
Closed

https: refactor to use http internals #16395

wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Oct 23, 2017

Rather than using http, use _http_client, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

https

@nodejs-github-bot nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Oct 23, 2017
@bengl
Copy link
Member Author

bengl commented Oct 23, 2017

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

@bengl
Copy link
Member Author

bengl commented Oct 23, 2017

/cc @nodejs/http

apapirovski
apapirovski approved these changes Oct 23, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

👍 LGTM. One tiny (totally optional) nit.

lib/https.js Outdated
const util = require('util');
const agent = require('_http_agent');
const server = require('_http_server');
Copy link
Member

@apapirovski apapirovski Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just a nit but could these be:

const { Agent: HttpAgent } = require('_http_agent');
const {
  Server: HttpServer,
  _connectionListener: httpConnectionListener
} = require('_http_server');

The usage further down feels a bit clearer that way Server.prototype.setTimeout = HttpServer.prototype.setTimeout;

cjihrig
cjihrig approved these changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Code LGTM, but might need to consider semverness if monkey patching code could be impacted.

lib/https.js
@@ -237,11 +235,20 @@ exports.request = function request(options, cb) {
options = util._extend({}, options);
}
options._defaultAgent = globalAgent;
return http.request(options, cb);
};
return new ClientRequest(options, cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this PR break existing code that relies on monkey patching?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely a good concern. I'm not immediately aware of modules monkeypatching the client side code but it is definitely something we need to look for before landing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

If anyone is monkey patching http.request, then http.ClientRequest and _http_client.ClientRequest also need to be monkey patched since they're all exposed and equivalent. I've seen userland code using all three.

That https.request currently calls http.request is an implementation detail (i.e. semver-patch). Users should not be relying on that.

Copy link

Choose a reason for hiding this comment

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

Just found this commit after noticing our tests for https break. Turns out our instrumentation relied on this implementation detail. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be breaking GCP's Stackdriver Trace as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

revert for v8.x in #16660

@bengl
https: refactor to use http internals
254aa3f 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.
@bengl
Copy link
Member Author

bengl commented Oct 25, 2017

@apapirovski Your nit is now addressed.

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

@jasnell
Copy link
Member

jasnell commented Oct 25, 2017

I'd like to see a CITGM run and some verification that express and hapi still work with this.

@bengl
Copy link
Member Author

bengl commented Oct 25, 2017

@jasnell Here's a CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1027/

Also I just ran npm it on express and hapi with this patch, all seemed good.

@jasnell
Copy link
Member

jasnell commented Oct 25, 2017

Awesome. Thanks!

bengl added a commit that referenced this pull request Oct 26, 2017
@bengl
https: refactor to use http internals
5118f31 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: #16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bengl
Copy link
Member Author

bengl commented Oct 26, 2017

Landed in 5118f31

@bengl bengl closed this Oct 26, 2017
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@bengl @gibfahn
https: refactor to use http internals
e494e91 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: #16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@bengl @gibfahn
https: refactor to use http internals
1a0e246 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: #16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@bengl @gibfahn
https: refactor to use http internals
754df71 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: #16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
@kjin kjin mentioned this pull request Nov 1, 2017
Closed
@MylesBorins MylesBorins mentioned this pull request Nov 1, 2017
Closed
MylesBorins added a commit to MylesBorins/node that referenced this pull request Nov 1, 2017
@MylesBorins
Revert "https: refactor to use http internals"
230352c 
This reverts commit 5118f31.

It is breaking code in the wild that depends on the original behavior
to do tracing.

I don't think we need to necessarily fix this in 8.x but we might want
to reclassify the original commit as Semver Major

Refs: nodejs#16395
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Nov 1, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@bengl @Qard
https: refactor to use http internals
f639f88 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: nodejs/node#16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@bengl @Qard
https: refactor to use http internals
e1e6b0c 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: nodejs/node#16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 3, 2017
@MylesBorins
Revert "https: refactor to use http internals"
08b75c1 
This reverts commit 5118f31.

It is breaking code in the wild that depends on the original behavior
to do tracing.

I don't think we need to necessarily fix this in 8.x but we might want
to reclassify the original commit as Semver Major

PR-URL: #16660
Refs: #16395
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@bengl @addaleax
https: refactor to use http internals
96eb4d7 
Rather than using `http`, use `_http_client`, etc. directly.

Also moving all the exports to the bottom, in line with most of the rest
of the codebase.

PR-URL: nodejs/node#16395
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots ofrobots left review comments

@NatalieWolfe NatalieWolfe NatalieWolfe left review comments

@MylesBorins MylesBorins MylesBorins left review comments

@apapirovski apapirovski apapirovski approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
https Issues or PRs related to the https subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bengl @jasnell @apapirovski @ofrobots @NatalieWolfe @MylesBorins @lpinca @cjihrig @nodejs-github-bot