GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: support overriding http\s.globalAgent by illBeRoy · Pull Request #25170 · nodejs/node

Conversation

@illBeRoy

@illBeRoy illBeRoy commented Dec 21, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss, juanarbol, and xardbaiz reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@nodejs-github-bot nodejs-github-bot added http Issues and PRs related to the http subsystem. https Issues and PRs related to the https subsystem. labels Dec 21, 2018
@illBeRoy
lib: support overriding http\s.globalAgent
633e0fe
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
@illBeRoy
illBeRoy force-pushed the fix/overridable-http-https-global-agent branch from 23a222e to 633e0fe Compare December 21, 2018 20:13
@illBeRoy illBeRoy mentioned this pull request Dec 21, 2018
Can we restart the work on overridable globalAgent? #23281
Closed
@Trott

Trott commented Dec 23, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 23, 2018

Copy link
Copy Markdown
Member

Will this require updates to the docs?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@illBeRoy

illBeRoy commented Dec 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 23, 2018

Copy link
Copy Markdown
Member

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19764/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Resume Build CI again: https://ci.nodejs.org/job/node-test-pull-request/19768/ ✔️

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 23, 2018

Copy link
Copy Markdown
Member

@nodejs/http

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 27, 2018
jasnell
jasnell approved these changes Dec 27, 2018
View reviewed changes
@illBeRoy

illBeRoy commented Jan 7, 2019

Copy link
Copy Markdown
Contributor Author

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Jan 7, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

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

illBeRoy, chrmarti, and poseidonCore reacted with hooray emoji
All reactions
  • 🎉 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax closed this Jan 7, 2019
addaleax pushed a commit that referenced this pull request Jan 7, 2019
@illBeRoy @addaleax
lib: support overriding http\s.globalAgent
5774688
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 <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 9, 2019
@illBeRoy @addaleax
lib: support overriding http\s.globalAgent
8d0c638
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 <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@illBeRoy @refack
lib: support overriding http\s.globalAgent
8960028
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 <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
v11.7.0 proposal #25537
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@illBeRoy @BridgeAR
lib: support overriding http\s.globalAgent
16e28eb
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 <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@BridgeAR
2019-01-17, Version 11.7.0 (Current), @BridgeAR
6b7b827
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
@BridgeAR
2019-01-17, Version 11.7.0 (Current), @BridgeAR
d3f8f90
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 mentioned this pull request Jan 24, 2019
v11.8.0 proposal #25687
Merged
This was referenced Mar 16, 2019
HTTP and HTTPS agent node-fetch/node-fetch#588
Closed
Cross-protocol redirect reuses same agent, but for instance http.Agent doesn't support followed https request node-fetch/node-fetch#571
Closed
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@illBeRoy @BethGriggs
lib: support overriding http\s.globalAgent
43dd99c
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 <[email protected]>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
v10.16.0 proposal #27514
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@illBeRoy @BethGriggs
lib: support overriding http\s.globalAgent
5bd6fe7
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@illBeRoy @MylesBorins
lib: support overriding http\s.globalAgent
73230cc
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 <[email protected]>
@ktrongnhan ktrongnhan mentioned this pull request May 26, 2019
Node 10.8.0 does not support lookup on Agent #27695
Closed
This was referenced May 29, 2019
2019-05-28 Version 10.16.0 'Dubnium' (LTS) @BethGriggs beevelop/docker-android-nodejs#315
Closed
2019-05-28 Version 10.16.0 'Dubnium' (LTS) @BethGriggs beevelop/docker-android-nodejs#316
Closed
2019-05-28 Version 10.16.0 'Dubnium' (LTS) @BethGriggs beevelop/docker-android-nodejs#317
Closed
2019-05-28 Version 10.16.0 'Dubnium' (LTS) @BethGriggs beevelop/docker-android-nodejs#318
Closed
2019-05-28 Version 10.16.0 'Dubnium' (LTS) @BethGriggs beevelop/docker-android-nodejs#319
Closed
@whut whut mentioned this pull request Jul 17, 2023
https: make https.globalAgent overridable also under ECMAScript Modules #48808
Closed
whut added a commit to whut/node that referenced this pull request Jul 20, 2023
@whut
https: make https.globalAgent overridable also under ECMAScript Modules
c1381e0
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem. https Issues and PRs related to the https subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can we restart the work on overridable globalAgent?

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@illBeRoy @Trott @addaleax @jasnell @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.