Uh oh!
There was an error while loading. Please reload this page.
http: make globalAgent of http and https as overridable properties - #9386
http: make globalAgent of http and https as overridable properties#9386diorahman wants to merge 1 commit into
globalAgent of http and https as overridable properties#9386Conversation
The `http.globalAgent` and `https.globalAgent` previously cannot be overridden directly by reassigning new a value. By exposing the `globalAgent`s of `http` and `https`, now it can be reassigned by: http.globalAgent = new MyAgent(); https.globalAgent = new MySecureAgent();
sam-github
commented
Oct 31, 2016
I would include docs in the PR, because the docs will show how the API changes, and let you document the usefulness of it. Also, does this really work? What happens to any requests that are in process and using the old global agent? And to its cached connections? Do they just leak, or do they timeout and go away? I'm also pretty surprised to see that https.request seems to be modifying its What about just making the globalAgent propert read-only, so nobody can try to assign to it by mistake? |
Trott
commented
Oct 31, 2016
Can you add a test? See https://github.com/nodejs/node/blob/master/doc/guides/writing_tests.md and maybe try to find a similar test in |
sam-github
commented
Oct 31, 2016
@Trott I suspect that @diorahman deliberately didn't test or doc, because it is not sure people are 👍 or 👎 on the entire idea. Which is reasonable, IMO. I suggested docs because I think its hard to evaluate the reasonableness of API changes that are doc-less. That may just be me. |
| const ClientRequest = exports.ClientRequest = client.ClientRequest; | ||
| exports.request = function request(options, cb) { | ||
| options._defaultAgent = options._defaultAgent || exports.globalAgent || agent.globalAgent; |
There was a problem hiding this comment.
Long line. make test would have caught this, it runs make lint.
diorahman
commented
Nov 1, 2016
@sam-github@Trott yes, as Sam said (and it is outlined in the #9057) the idea is still unclear. Will try to provide tests and experiment with what suggested by @sam-github in #9386 (comment) |
jasnell
commented
Nov 2, 2016
@nodejs/http ... thoughts? @diorahman ... would you mind adding a test? |
lance
commented
Jan 19, 2017
This seems to have been dropped. Is there any reason to push it forward or can it be closed along with #9057? |
bnoordhuis
commented
Jan 19, 2017
I'll go ahead and close it. Can reopen if @diorahman wants to follow through. |
Under ECMAScript modules when you do "import * as https from 'https'" you get a new object with properties copied from https module exports. So if this is a regular data property, you will just override a copy, but if this would be a accessor property, we can still access the actual https.globalAgent. Refs: nodejs#25170, nodejs#9386
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
http, https
Description of change
The
http.globalAgentandhttps.globalAgentpreviously cannot be overridden directly by reassigning new a value.By exposing the
globalAgents ofhttpandhttps, now it can be reassigned by:Fix#9057