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.

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

lib: support overriding http\s.globalAgent #25170

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@illBeRoy
Copy link
Contributor

illBeRoy commented Dec 21, 2018

Overriding require('http').globalAgent and require('https').globalAgent is now respected by consequent requests.

In order to achieve that, the following changes were made:

  1. http: module.exports.globalAgent is now defined through Object.defineProperty. Its getter and setter return \ set require('_http_agent').globalAgent.
  2. https: the https globalAgent is not the same as _http_agent, and is defined in https module itself. Therefore, the fix here was to simply use module.exports.globalAgent to support mutation.
  3. test/parallel: according tests were added for both http and https, where in both we create a server, set the default agent to a newly created instance and make a request to that server. We then assert that the given instance was actually used by inspecting its sockets property.

Fixes: #23281

On a side note, this is my first contribution to the project. Would appreciate feedback :)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
lib: support overriding http\s.globalAgent
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.

In order to achieve that, the following changes were made:

1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.

2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.

3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.

Fixes: #23281

@illBeRoy illBeRoy force-pushed the illBeRoy:fix/overridable-http-https-global-agent branch from 23a222e to 633e0fe Dec 21, 2018

@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 23, 2018

Will this require updates to the docs?

@illBeRoy

This comment has been minimized.

Copy link
Contributor Author

illBeRoy commented Dec 23, 2018

Hey @Trott,

I believe that the way it worked up until now was a bug, because one could expect reassigning to http[s] globalAgent would work much like mutating it, which currently works. The inconsistent behavior, where mutating is possible but overriding is not, was the result of destructuring in both modules, and that is what I attended to :)

That's why I don't think a change to the docs is required - as far as I can see, they simply state that the globalAgent is the default agent for all requests (in both HTTP and HTTPS).

Then again - if you deem it required, I'll be more than happy update my PR :)

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 23, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 23, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 23, 2018

@illBeRoy

This comment has been minimized.

Copy link
Contributor Author

illBeRoy commented Jan 7, 2019

Hey again :) I'm not quite sure about the process, but since it's approved, is there anything else for me to do? how do I know if it was merged? Thanks!

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 7, 2019

@illBeRoy Pinging us is the exact right thing to do. :)

Landed in 5774688, and congratulations to your first contribution! 🎉

@addaleax addaleax closed this Jan 7, 2019

addaleax added a commit that referenced this pull request Jan 7, 2019

lib: support overriding http\s.globalAgent
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.

In order to achieve that, the following changes were made:

1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.

2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.

3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.

Fixes: #23281

PR-URL: #25170
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Jan 9, 2019

lib: support overriding http\s.globalAgent
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.

In order to achieve that, the following changes were made:

1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.

2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.

3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.

Fixes: #23281

PR-URL: #25170
Reviewed-By: James M Snell <jasnell@gmail.com>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

lib: support overriding http\s.globalAgent
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.

In order to achieve that, the following changes were made:

1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.

2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.

3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.

Fixes: nodejs#23281

PR-URL: nodejs#25170
Reviewed-By: James M Snell <jasnell@gmail.com>

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

lib: support overriding http\s.globalAgent
Overriding `require('http[s]').globalAgent` is now respected by
consequent requests.

In order to achieve that, the following changes were made:

1. Implmentation in `http`: `module.exports.globalAgent` is now defined
through `Object.defineProperty`. Its getter and setter return \ set
`require('_http_agent').globalAgent`.

2. Implementation in `https`: the https `globalAgent` is not the same
as `_http_agent`, and is defined in `https` module itself. Therefore,
the fix here was to simply use `module.exports.globalAgent` to support
mutation.

3. According tests were added for both `http` and `https`, where in
both we create a server, set the default agent to a newly created
instance and make a request to that server. We then assert that the
given instance was actually used by inspecting its sockets property.

Fixes: nodejs#23281

PR-URL: nodejs#25170
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006

PR-URL: nodejs#25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) nodejs#25361

PR-URL: nodejs#25537

BridgeAR added a commit that referenced this pull request Jan 18, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    #24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    #24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    #25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    #25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    #22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    #25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    #25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) #25361

PR-URL: #25537

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment