Skip to content
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

Modify https.globalAgent doesn't effect #9057

Closed
vagruchi opened this issue Oct 12, 2016 · 10 comments
Closed

Modify https.globalAgent doesn't effect #9057

vagruchi opened this issue Oct 12, 2016 · 10 comments
Labels
good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem.

Comments

@vagruchi
Copy link

  • Version: 6.5.0
  • Platform: Linux 3.10.0-327.13.1.el7.x86_64
  • Subsystem: https

I modify globalAlgent to use proxy.
Next code:

var https = require("https")
var url = require("url")

https.globalAgent = MyProxyAgent
let parsedURL = url.parse(urlString)
parsedURL.method = 'POST' 
let req = https.request(parsedURL, console.log)
req.on("error", console.log)
req.end()

this give me error:
Error: connect ECONNREFUSED
But if i paste this code parsedURL.agent = https.globalAgent before https.request all works fine.
Also i tried to modify http.globalAgent. It isn't work.

@bnoordhuis bnoordhuis added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Oct 12, 2016
@bnoordhuis
Copy link
Member

http.globalAgent and https.globalAgent are not currently overridable properties. That is, you can reassign them but the actual global agent lives elsewhere.

Some discussion about the ramifications are probably warranted. I think your best bet would be to open a pull request and see how it is received.

@bnoordhuis bnoordhuis added the good first issue Issues that are suitable for first-time contributors. label Oct 21, 2016
@bnoordhuis
Copy link
Member

I'm adding the 'good first contribution' label but if no one picks it up in the next week or two, I'm going to close it out.

@diorahman
Copy link

diorahman commented Oct 30, 2016

Hi @bnoordhuis, should the patch lets http.globalAgent and https.globalAgent to be reassigned? Will try to make a PR as my first contribution.

@bnoordhuis
Copy link
Member

@diorahman That's the idea, yes.

@shubheksha
Copy link
Contributor

Can I take this up as my first contribution in case no one is working on it anymore?

@addaleax
Copy link
Member

@shubheksha I don’t think anybody’s working on it – so, yes, go for it!

There’s a bit of information on the process in our CONTRIBUTING.md and there’s #node-dev on chat.freenode.net where you can ask any questions about contributing that you have.

For this specific task I would suggest that you comment here with an outline of the changes that you are going to make, if that’s possible. (At least that’s what I would do, because I am not very sure just from reading the issue description.)

@shubheksha
Copy link
Contributor

@addaleax, I understand what needs to be done but I'm not sure how to go about this. Will it be possible to give me some pointers regarding where to start?

@sotayamashita
Copy link
Contributor

@shubheksha You can understand the issue to see #9386

@shubheksha
Copy link
Contributor

@sotayamashita, I went through that but it didn't make a lot of sense. However, @Fishrock123 helped me in understanding it piece by piece so I can place things now. 😄

@addaleax & @bnoordhuis: is there a reason why the require statements use const and everything else uses var? Is it okay if I replace it with let or const?

@addaleax
Copy link
Member

addaleax commented Feb 8, 2017

is there a reason why the require statements use const and everything else uses var? Is it okay if I replace it with let or const?

Right now, we don’t do these bulk replacements in lib/ for two reasons:

  • It’s a lot of churn and prone to creating merge conflicts (for open PRs or when backporting changes to older release lines), and kind of breaks git blame
  • At least for the past it has been true that var in loops is faster than let (I know, it sounds weird… 😄)

You can feel free to do varconst replacements in the parts of the code that you directly touch while putting your PR together, and you can use let and const for any test files that you change/create/…

Hope that helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants