improve proxy bypass on Windows #3979

schlamar · 2017-04-20T13:42:43Z

codecov-io · 2017-04-20T13:52:18Z

Codecov Report

Merging #3979 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3979      +/-   ##
==========================================
- Coverage   89.76%   89.74%   -0.03%     
==========================================
  Files          15       15              
  Lines        1955     1940      -15     
==========================================
- Hits         1755     1741      -14     
+ Misses        200      199       -1

Impacted Files	Coverage Δ
requests/structures.py	`100% <ø> (ø)`	⬆️
requests/compat.py	`100% <100%> (ø)`	⬆️
requests/utils.py	`86.25% <100%> (+1.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26ab951...5c389cf. Read the comment docs.

schlamar · 2017-04-20T14:06:13Z

As first step I added tests to check the current behavior (they take around 30 seconds to run on my system) so we can detect regressions in the new implementation.

Tested with Python 2.7 and 3.5.

schlamar · 2017-04-20T14:33:54Z

Well coverage drops obviously as these tests are only run on Windows :-/

From my POV this is ready to get merged, please review.

Lukasa

Hrm, we may need to enable appveyor to ensure that this stuff actually works.

Lukasa · 2017-04-20T14:57:03Z

requests/utils.py

+                                              'ProxyEnable')[0]
+            proxyOverride = str(winreg.QueryValueEx(internetSettings,
+                                                    'ProxyOverride')[0])
+            # ^^^^ Returned as Unicode but problems if not converted to ASCII


This comment is confusing: what is it trying to say?

Lukasa · 2017-04-20T14:57:48Z

requests/utils.py

+            for val in host:
+                if re.match(test, val, re.I):
+                    return 1
+        return 0


We may as well just use booleans.

Lukasa · 2017-04-20T14:59:32Z

requests/utils.py

+
+        # try to make a host list from name and IP address.
+        rawHost, port = splitport(host)
+        host = [rawHost]


What is this for?

Lukasa · 2017-04-20T15:00:55Z

requests/utils.py

+        # canonical entry.
+        proxyOverride = proxyOverride.split(';')
+        # now check if we match one of the registry values.
+        for test in proxyOverride:


We can probably unify these two lines.

Lukasa · 2017-04-20T15:06:11Z

tests/test_utils.py

+    if compat.is_py3:
+        import winreg
+    else:
+        import _winreg as winreg


May as well just import this directly from compat.

Then we would need a platform check in compat and winreg could be None on non Windows platforms...

schlamar · 2017-04-20T15:45:28Z

The code is copy pasted from Python 3.5's urlib/request.py with the lines doing DNS lookups (and appending to host) removed. I would recommend leaving it as is (maybe add a clarifying comment about this).

Lukasa · 2017-04-20T17:06:58Z

I'm a bit disinclined to do that unless we think this is going to be temporary code (which it won't be until we drop 2.7 support at least). If we are going to support it I'd like the code to be sensible and comprehensible. 😃

schlamar · 2017-05-02T06:42:26Z

@Lukasa Please give me a note when AppVeyor tests are working. Then I'll udpate this PR.

schlamar · 2017-05-03T13:38:33Z

@Lukasa Should I revert c121b98? With this PR there is no reason anymore for caching the results...

Lukasa · 2017-05-03T13:39:39Z

@schlamar Yeah, might be worth doing.

kennethreitz · 2017-05-03T17:56:59Z

close/reopened to get appveyor to re-run

Lukasa · 2017-05-03T17:58:04Z

It'll need to be rebased on top of master.

nateprewitt

This looks like a good start @schlamar! I left a couple notes with some outstanding questions from my end. A couple are just stylistic nits but the others are on functionality.

nateprewitt · 2017-05-03T18:26:20Z

requests/utils.py

+        for test in proxyOverride:
+            if test == '<local>':
+                if '.' not in host:
+                    return 1


Is there a reason this is 1 and not True like the other return values? If so, we should probably add a comment explaining why.

Nope, I forgot this one 🙈

nateprewitt · 2017-05-03T18:29:56Z

requests/compat.py


 if is_py2:
-    from urllib import quote, unquote, quote_plus, unquote_plus, urlencode, getproxies, proxy_bypass
+    from urllib import quote, unquote, quote_plus, unquote_plus, urlencode, getproxies, proxy_bypass, \


It may be better to follow something closer to the import convention of the other modules, rather than use \.

from urllib import ( quote, unquote, quote_plus, unquote_plus, urlencode, getproxies, proxy_bypass, proxy_bypass_environment, getproxies_environment)

nateprewitt · 2017-05-03T18:35:26Z

requests/utils.py

+
+        Returns settings gathered from the environment, if specified,
+        or the registry.
+


Could we remove this extra line here.

nateprewitt · 2017-05-03T18:38:06Z

requests/utils.py

+        if getproxies_environment():
+            return proxy_bypass_environment(host)
+        else:
+            return proxy_bypass_registry(host)


This seems to return a boolean but the docstring states it returns a dictionary of scheme to proxy mappings. It may be a good idea to clarify this case.

Somehow I copied the wrong docstring, I'll fix this :)

schlamar · 2017-05-04T06:11:14Z

@Lukasa @nateprewitt Updated, please review again.

schlamar · 2017-05-04T06:37:22Z

BTW, coverage drops because reverting 8e6e47a and c121b98 removes a lot of covered lines. My changes alone do not negatively affect coverage.

Lukasa · 2017-05-04T07:14:19Z

@schlamar Based on codecov's output the actual reason you're reducing coverage is that the diff isn't covered enough to avoid regressing it. To match coverage you'd need to cover 90% of the diff, but only 86% is covered. You can see the lines we're missing here: what are the odds that we can write tests that appropriately cover them?

Lukasa

Cool, so this is looking really good! I'd love to see if we can get the test coverage up a bit higher, and I've left a note in the diff. 😄

Lukasa · 2017-05-04T07:15:27Z

requests/utils.py

 DEFAULT_CA_BUNDLE_PATH = certs.where()


+if os.name == 'nt':


Rather than use os.name, might it be better to use platform.system?

schlamar · 2017-05-04T08:13:27Z

requests/utils.py

+                import _winreg as winreg
+
+        except ImportError:
+            # Std modules, so should be around - but you never know!


@Lukasa Should I remove this one, I don't think this is really necessary...

Yeah, let's do that: there's no need to tolerate this ImportError I don't think.

schlamar · 2017-05-04T11:29:06Z

@Lukasa Updated, please review again :)

Lukasa

Ok, I think I'm happy. Time to see what @nateprewitt thinks.

nateprewitt

Looks good to me!

Lukasa

Ok then, one last request. Can you add notes to the changelog to note that a) we've changed our strategy for looking up proxy bypass information on Windows, and b) we've removed the proxy bypass cache as it's no longer needed? Just want to make sure we have a good reference in case these things have unforseen bugs, and I bet I'll forget that there were two things in this PR when it comes to writing the changelog later. 😁

schlamar · 2017-05-04T13:26:10Z

When was the proxy bypass cache introduced? I'd like to make a reference but can't find anything in the changelog :)

nateprewitt · 2017-05-04T13:28:34Z

@schlamar, I believe it was only a couple months ago (#3885).

schlamar · 2017-05-04T13:31:02Z

Ah yes, this wasn't even released yet. @Lukasa in this case I would ignore this?!

Lukasa · 2017-05-04T13:31:56Z

Yeah, you would. =)

Lukasa

Alright, let's do it!

sethmlarson · 2017-05-04T14:32:27Z

🎉

schlamar force-pushed the bypass-proxy branch from d7b4a95 to 61db48f Compare April 20, 2017 14:01

schlamar changed the title ~~[WIP] improve proxy bypass on Windows~~ improve proxy bypass on Windows Apr 20, 2017

Lukasa requested changes Apr 20, 2017

View reviewed changes

Lukasa mentioned this pull request Apr 21, 2017

Add appveyor testing of Requests. #3983

Merged

schlamar force-pushed the bypass-proxy branch 2 times, most recently from 7d563d4 to db57972 Compare May 3, 2017 14:23

kennethreitz closed this May 3, 2017

kennethreitz reopened this May 3, 2017

nateprewitt requested changes May 3, 2017

View reviewed changes

schlamar added 2 commits May 4, 2017 07:46

revert 8e6e47a and c121b98

724fd44

test proxy bypass with config from registry

4f34446

schlamar force-pushed the bypass-proxy branch from db57972 to 32a3472 Compare May 4, 2017 05:55

Lukasa requested changes May 4, 2017

View reviewed changes

schlamar commented May 4, 2017

View reviewed changes

schlamar force-pushed the bypass-proxy branch from 32a3472 to aa063fe Compare May 4, 2017 08:15

proxy bypass on Windows without DNS lookups

1c38e1f

schlamar force-pushed the bypass-proxy branch from aa063fe to 1c38e1f Compare May 4, 2017 08:44

schlamar closed this May 4, 2017

schlamar reopened this May 4, 2017

Lukasa approved these changes May 4, 2017

View reviewed changes

nateprewitt approved these changes May 4, 2017

View reviewed changes

Lukasa requested changes May 4, 2017

View reviewed changes

update changelog

5c389cf

Lukasa approved these changes May 4, 2017

View reviewed changes

Lukasa merged commit 216aee4 into psf:master May 4, 2017

schlamar mentioned this pull request May 4, 2017

Don't use urllib.proxy_bypass on Windows #3976

Closed

schlamar deleted the bypass-proxy branch May 5, 2017 13:38

florimondmanca mentioned this pull request Oct 13, 2019

should-b-proxied: logic whether url to be proxied encode/httpx#472

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2021


		Returns settings gathered from the environment, if specified,
		or the registry.

Uh oh!

improve proxy bypass on Windows #3979

improve proxy bypass on Windows #3979

Uh oh!

Conversation

schlamar commented Apr 20, 2017

Uh oh!

codecov-io commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schlamar commented Apr 20, 2017

Uh oh!

schlamar commented Apr 20, 2017

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlamar commented Apr 20, 2017

Uh oh!

Lukasa commented Apr 20, 2017

Uh oh!

schlamar commented May 2, 2017

Uh oh!

schlamar commented May 3, 2017

Uh oh!

Lukasa commented May 3, 2017

Uh oh!

kennethreitz commented May 3, 2017

Uh oh!

Lukasa commented May 3, 2017

Uh oh!

nateprewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nateprewitt May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlamar commented May 4, 2017

Uh oh!

schlamar commented May 4, 2017

Uh oh!

Lukasa commented May 4, 2017

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlamar commented May 4, 2017

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 20, 2017 •

edited

Loading

nateprewitt May 3, 2017 •

edited

Loading