https: refactor to use http internals by bengl · Pull Request #16395 · nodejs/node · GitHub

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

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

/cc @nodejs/http

👍 LGTM. One tiny (totally optional) nit.

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;

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

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

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.

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.

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

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

revert for v8.x in #16660

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.

@apapirovski Your nit is now addressed.

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

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

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

Awesome. Thanks!

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>

Landed in 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>

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

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>

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>

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>

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

apapirovski approved these changes Oct 23, 2017

View reviewed changes

cjihrig approved these changes Oct 23, 2017

View reviewed changes

lpinca approved these changes Oct 24, 2017

View reviewed changes

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 force-pushed the bengl/refactorhttps branch from a3cedb1 to 254aa3f Compare October 25, 2017 04:26

jasnell approved these changes Oct 25, 2017

View reviewed changes

bengl closed this Oct 26, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

kjin mentioned this pull request Nov 1, 2017

https.request doesn't work in Node 8.9+ googleapis/cloud-trace-nodejs#589

Closed

MylesBorins mentioned this pull request Nov 1, 2017

[v8.x] https: revert "refactor to use http internals" #16660

Closed

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

MylesBorins added dont-land-on-v6.x labels Nov 16, 2017

https: refactor to use http internals by bengl · Pull Request #16395 · nodejs/node · GitHub

Uh oh!

Uh oh!

https: refactor to use http internals - #16395

https: refactor to use http internals#16395bengl wants to merge 1 commit intonodejs:masternodejs/node:masterfrom bengl:bengl/refactorhttpsbengl/node:bengl/refactorhttpsCopy head branch name to clipboard

Conversation

bengl commented Oct 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

bengl commented Oct 23, 2017

Uh oh!

bengl commented Oct 23, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski Oct 23, 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!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

bengl Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

NatalieWolfe Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

ofrobots Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

MylesBorins Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

bengl commented Oct 25, 2017

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

bengl commented Oct 25, 2017

Uh oh!

jasnell commented Oct 25, 2017

Uh oh!

bengl commented Oct 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

https: refactor to use http internals#16395
bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:bengl/refactorhttps

apapirovski Oct 23, 2017 •
edited
Loading