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

Proxy selector #1062

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Proxy selector #1062

wants to merge 8 commits into from

Conversation

technoweenie
Copy link
Member

I started poking at the proxy code, and then decided trying to implement the net/http/httpproxy package in Go. Basically, I thought what it'd be like to replace 14 lines of code with my own version in 326 lines (plus docs):

if URI.parse('').respond_to?(:find_proxy)
case url
when String
uri = Utils.URI(url)
uri = URI.parse("#{uri.scheme}://#{uri.hostname}").find_proxy
when URI
uri = url.find_proxy
when nil
uri = find_default_proxy
end
else
warn 'no_proxy is unsupported' if ENV['no_proxy'] || ENV['NO_PROXY']
uri = find_default_proxy
end

Three benefits:

  1. no_proxy is correctly handled in cases where URI#find_proxy don't exist. I don't know what versions are affected, so this may be a moot point.
  2. Potential speed improvements on repeat #proxy_for calls, especially with lots of no_proxy entries.
  3. Actually support https_proxy

Is it worth it? I'm not really sure. But it was fun to write :)

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth it? I'm not really sure. But it was fun to write :)

I just hope you don't expect me to maintain this 😂. Wouldn't know where to start from!

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah for a moment I thought this was going to be Go code 🙄!
Looks good, but why not relying on the out-of-the-box method provided by Ruby? What are the main differences?

@technoweenie
Copy link
Member Author

I listed the 3 benefits in the original PR message above. Basically, the code more correctly implements the environment vars, and it should be more efficient in long running scripts with the way it checks uri hosts against no_proxy.

I'm okay leaving this PR here until we're sure we want it. I extracted #1063 out of it too.

@iMacTia
Copy link
Member

iMacTia commented Oct 30, 2019

I have no doubts this can actually be better than the Ruby solution, I'm just a bit unconfident with reinventing the wheel (and then maintaining it). Is there any chance we can propose a fix on the Ruby implementation itself? Too audacious?

@iMacTia iMacTia added the parked Parked for future label Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parked Parked for future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants