https: refactor to use http internals#16395
Conversation
|
/cc @nodejs/http |
apapirovski
left a comment
There was a problem hiding this comment.
👍 LGTM. One tiny (totally optional) nit.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Code LGTM, but might need to consider semverness if monkey patching code could be impacted.
There was a problem hiding this comment.
Could this PR break existing code that relies on monkey patching?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just found this commit after noticing our tests for https break. Turns out our instrumentation relied on this implementation detail. :)
There was a problem hiding this comment.
It seems to be breaking GCP's Stackdriver Trace as well.
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.
a3cedb1 to
254aa3f
Compare
|
@apapirovski Your nit is now addressed. |
|
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 |
|
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>
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>
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>
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>
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), orvcbuild test(Windows) passesAffected core subsystem(s)
https