https: refactor to use http internals#16395
Uh oh!
There was an error while loading. Please reload this page.
Conversation
bengl commented Oct 23, 2017
bengl commented Oct 23, 2017
/cc @nodejs/http |
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
👍 LGTM. One tiny (totally optional) nit.
lib/https.js Outdated
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
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.
lib/https.js Outdated
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.
benglforce-pushed the bengl/refactorhttps branch from a3cedb1 to 254aa3fComparebengl commented Oct 25, 2017
@apapirovski Your nit is now addressed. |
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 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 |
jasnell commented Oct 25, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
bengl commented Oct 26, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 added baking-for-lts 
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Jan Krems <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
9 participants
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