Cache proxy bypass #3885

dfontenot · 2017-02-22T04:21:30Z

Added simple cache over urllib.proxy_bypass to remedy #2988. Holds on to entries for a minute. Once it reaches its max size, will evict the first entry that is the oldest.

Added tests over the structure, did not add a test that should_bypass_proxies invokes the cache.

This is my first pull request; please let me know if I've overlooked something.

Additionally, when I check out master, run make coverage in pipenv, the test named 'TestRequests.test_proxy_error' fails. This is still the only failing test when I use my branch. The error is ConnectionError: ('Connection aborted.', BadStatusLine("''",)). Is there some additional setup I need to do to run the test suite?

codecov-io · 2017-02-22T05:12:21Z

Codecov Report

Merging #3885 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3885      +/-   ##
==========================================
+ Coverage   89.08%   89.32%   +0.24%     
==========================================
  Files          15       15              
  Lines        1896     1940      +44     
==========================================
+ Hits         1689     1733      +44     
  Misses        207      207

Impacted Files	Coverage Δ
requests/utils.py	`85.02% <100%> (+0.08%)`	✅
requests/structures.py	`100% <100%> (ø)`	✅

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 d6f4818...c121b98. Read the comment docs.

Lukasa

Cool, so this is a good start! I've left some notes inline in the diff.

Lukasa · 2017-02-22T08:33:10Z

requests/structures.py

@@ -103,3 +104,94 @@ def __getitem__(self, key):

    def get(self, key, default=None):
        return self.__dict__.get(key, default)
+
+
+class TimedCache(object):


So, this is basically a collections.MutableMapping! We should make it actually be one, because that gives us a number of handy-dandy convenience methods.

Will update to have TimedCache inherit collections.MutableMapping

Lukasa · 2017-02-22T08:34:08Z

requests/structures.py

+        :return: the value in the cache, or None
+        """
+
+        return self.get(key, None)


So, generally speaking it is better to implement get in terms of __getitem__ than the other way around. __getitem__ should almost always maintain dict-like behaviour of raising KeyError if the entry is not present, rather than returning None. The core concern here is that it needs to be possible to distinguish between "key not present in dictionary" and "key present with a value of None".

Will update.

Lukasa · 2017-02-22T08:35:42Z

requests/structures.py

+
+        found = self._dict.get(key, default)
+        if not found:
+            return default


This logic is wrong. Happily, it doesn't matter: if you make this a subclass of MutableMapping you don't actually need to implement this method at all, we get it for free.

Lukasa · 2017-02-22T08:37:18Z

requests/structures.py

+            return default
+
+        occurred, value = found
+        now = int(time.time())


So, this is a problem.

If possible we'd like to use time.monotonic instead, as time.time can go backwards. So I think in this module you'll need to feature detect on import to see whether time.monotonic is present and to prefer it if it is.

Thanks. To clarify, you're saying if time.monotonic is not present, it's ok to fall back to time.time?

Lukasa · 2017-02-22T08:37:55Z

requests/structures.py

+        :param value: the value to be added to the cache
+        """
+
+        now = int(time.time())


Same note about monotonic time.

Lukasa · 2017-02-22T08:38:16Z

requests/structures.py

+        Locates the value at lookup key,
+        if cache is full, will evict the first
+        found expired entry, otherwise will evict
+        the oldest non-expired item


You're wrapping this paragraph very short. Feel free to go all the way out to 89 columns!

Will update.

Lukasa · 2017-02-22T08:38:40Z

requests/structures.py

+        Locates the value at lookup key,
+        if cache is full, will evict the first
+        found expired entry, otherwise will evict
+        the oldest non-expired item


While we're here, can you add a blank line separating the paragraph blurb from the :param: directives?

Will update.

Lukasa · 2017-02-22T08:40:05Z

requests/structures.py

+        if len(self._dict) >= self.maxlen:
+            replacement_candidate_key = None
+            max_time_delta = 0
+            for cur_key, (occurred, _) in self._dict.items():


This complete search can probably be avoided by using an OrderedDict, can't it? urllib3 includes one that works on all the Python versions we support, so you can import that and use it as the base implementation to save ourselves this iterative walk.

Will replace self._dict with an OrderedDict.

Lukasa · 2017-02-22T08:41:31Z

requests/utils.py

+        PROXY_BYPASS_CACHE[netloc] = result
+        return result
+    else:
+        return cached_result


My suggestion is that this be reimplemented as a generic decorator, rather than written to specifically handle proxy_bypass. That way we get a much easier time of testing the timed cache and all the relevant code. =)

Lukasa · 2017-02-22T08:47:52Z

tests/test_structures.py

@@ -74,3 +74,56 @@ def test_getitem(self, key, value):
    @get_item_parameters
    def test_get(self, key, value):
        assert self.lookup_dict.get(key) == value
+
+
+class TestTimedCache:


Let's avoid making this an old-style class by putting (object) as well.

Will update.

dfontenot · 2017-02-22T20:59:35Z

@Lukasa I see a line like this already in the code base

current_time = getattr(time, 'monotonic', time.time)

as a top level definition. I've attempted to mock those, but my attempts so far have had some code smells. I've added a new commit that attempts to mock those in the least smelly way, but it's not ideal. Can you help me iron these out so they aren't so strange?

My approaches so far:

I set current_time in the class and mock it using mocker.patch.object. If I set it as a staticmethod or a classmethod, I'm not able to replace the function with a lambda w/o any args or with one anonymous arg. I'd rather not set it as an instance method of the class as that's a bit strange.
I use mocker.patch and set current_time at the global level of requests.structures. The only problem then is I have to import requests.structures (or just requests.structures.current_time), and pyflakes complains about an unused import.

Would appreciate any suggestions.

Lukasa · 2017-02-22T21:03:03Z

Your mocking will get a lot easier if you make the _current_time callable a module-level object rather than putting it at class scope. =)

dfontenot · 2017-02-22T21:19:42Z

I think I must have had a typo, mocker.patch with the fully-qualified package name for current_time works without doing any extra non-used imports.

Lukasa

Cool, this is generally looking really good: substantial progress! I've written some more notes.

Lukasa · 2017-02-23T09:06:25Z

requests/structures.py

+    """
+    Evicts entries after expiration_secs.
+    If none are expired and maxlen is hit,
+    will evict the oldest cached entry


You can have a slightly wider docstring than this! 😁

Lukasa · 2017-02-23T09:07:05Z

requests/structures.py

+        self._dict = OrderedDict()
+
+    def __repr__(self):
+        return '<timed-cache maxlen:%d len:%d>' % \


This should be TimedCache, and it should also provide the expiration_secs.

Lukasa · 2017-02-23T09:09:23Z

requests/structures.py

+        try:
+            return self.__getitem__(key)
+        except KeyError:
+            return default


You don't need to implement this: collections.MutableMapping will do it for you.

Lukasa · 2017-02-23T09:10:33Z

requests/structures.py

+
+        if len(self._dict) >= self.maxlen:
+            for _ in range(len(self._dict) - self.maxlen + 1):
+                self._dict.popitem(last=False)


This can be more cleanly expressed as:

while len(self._dict) >= self.maxlen: self._dict.popitem(last=False)

Agreed. Updated.

Lukasa · 2017-02-23T09:11:54Z

requests/structures.py

+
+    def clear(self):
+        """Clears the cache"""
+        return self._dict.clear()


This one we also technically get for free from MutableMapping, but in this case we can do it much faster than the generic implementation so I'm happy to have this override.

Good to know. I've left the override intact.

Lukasa · 2017-02-23T09:14:49Z

tests/test_structures.py

+        assert fnc_stub.call_count is 1
+        assert len(cache) is 1
+
+        assert invocation_spy.call_count is 2


Heh, so this is a good test, but it mostly tests implementation details. For this, it's enough to just mock out the time and then write a small function that constantly returns an incrementing value, then wrap it in the decorator and use the timer to demonstrate the behaviour.

Generally speaking, avoiding mocks is a good thing wherever we can. =)

Ok. I've re-made the test more along these lines. Let me know what you think.

Lukasa

Awesome, really close now! A couple of minor stylistic notes and one request to expand a test slightly and we're good to go!

Lukasa · 2017-02-23T21:05:35Z

requests/structures.py

+        :param expiration_secs: the number of seconds to hold on
+        to entries
+        """
+


Really minor nit, we don't need this blank line.

Lukasa · 2017-02-23T21:07:24Z

requests/utils.py

+
+    :rtype: bool
+    """
+


Same again, don't need this blank line.

Lukasa · 2017-02-23T21:07:48Z

requests/utils.py

+    Looks for netloc in the cache,
+    if not found, will call proxy_bypass
+    for the netloc and store its result
+    in the cache


This again can be flowed out to column 79 as well.

Lukasa · 2017-02-23T21:10:24Z

tests/test_structures.py

+        first_result = some_method(1)
+        assert first_result is 1
+        second_result = some_method(1)
+        assert second_result is 1


Should also check that a different argument doesn't return a cached value.

dfontenot · 2017-02-23T21:15:26Z

@Lukasa thanks for your help with this. As for squashing up all of the commits this PR has accumulated--who should do that? Do the maintainers of this lib do that? Or I can squash locally and push to a new PR (or just force push).

Lukasa · 2017-02-23T21:52:09Z

If you squash locally and force push that's fine, GitHub will work it all out. 👍

Lukasa

Sorry, I missed some extra whitespace. 😔 Let's clear all of that up and then we will be good to go. 👍

Lukasa · 2017-02-23T21:54:49Z

requests/structures.py

+    """
+    Wrap a function call in a timed cache
+    """
+


Lukasa · 2017-02-23T21:55:07Z

requests/structures.py

+    Evicts entries after expiration_secs. If none are expired and maxlen is hit,
+    will evict the oldest cached entry
+    """
+


Lukasa · 2017-02-23T21:55:24Z

requests/structures.py

+        :param key: which entry to look up
+        :return: the value in the cache, or None
+        """
+


Lukasa · 2017-02-23T21:55:40Z

requests/structures.py

+        :param key: the key to search the cache for
+        :param value: the value to be added to the cache
+        """
+


Lukasa · 2017-02-23T21:56:00Z

tests/test_structures.py

+
+
+class TestTimedCache(object):
+


Lukasa · 2017-02-23T21:56:19Z

tests/test_structures.py

+
+
+class TestTimedCacheManagedDecorator(object):
+


Lukasa · 2017-02-23T21:56:33Z

tests/test_structures.py

+class TestTimedCacheManagedDecorator(object):
+
+    def test_caches_repeated_calls(self, mocker):
+


Used to alleviate long gethostbyaddr calls Made new TimedCache and decorator to wrap a function with a cache * Entries looked up older than a minute (default amount) are evicted. * When full, evicts the oldest entry

dfontenot · 2017-02-23T22:07:36Z

Force pushed commit addresses the whitespace issues

Lukasa

Cool, I'm happy with this! As this is a big-ish change in functionality, I'd like to give @sigmavirus24 a chance to review before I merge, but if he doesn't get time I'll merge it Monday. Great work @davidfontenot!

sigmavirus24

I'll submit these updates in a separate pull request

sigmavirus24 · 2017-03-04T14:55:32Z

requests/structures.py

+            (self.maxlen, len(self._dict), self.expiration_secs)
+
+    def __iter__(self):
+        return map(lambda kv: (kv[0], kv[1][1]), self._dict.items()).__iter__()


I'm curious why we don't just build a generator here.

map() is slow and lambda s are slow. This intentional on Guido's part.

Calling __iter__ is really poor form.

This would be far simpler and easier to read as

return ((key, value[1]) for key, value in self._dict.items())

sigmavirus24 · 2017-03-04T14:56:52Z

requests/structures.py

+        return map(lambda kv: (kv[0], kv[1][1]), self._dict.items()).__iter__()
+
+    def __delitem__(self, item):
+        return self._dict.__delitem__(item)


Why not write this as:

del self._dict[item]

__delitem__ doesn't return anything so there's no need to worry about returning something here. And it would avoid calling the dunder method directly, which I'll repeat, is poor form.

sigmavirus24 · 2017-03-04T14:57:52Z

requests/structures.py

+
+        if now - occurred > self.expiration_secs:
+            del self._dict[key]
+            raise KeyError


This should raise a KeyError with the key as an argument, e.g., KeyError(key).

sigmavirus24 · 2017-03-04T14:58:54Z

requests/structures.py

+        while len(self._dict) >= self.maxlen:
+            self._dict.popitem(last=False)
+
+        return self._dict.__setitem__(key, (now, value))


Again, __setitem__ doesn't return anything. This should just be

self._dict[key] = (now, value)

There's no need to obfuscate any of this.

There were some odd decisions made about the implementation of some of the required methods for MuttableMapping in the TimedCache object. This cleans those up and makes the implementation, ever so slightly, easier to read. See also psf#3885

Lukasa requested changes Feb 22, 2017

View reviewed changes

Lukasa requested changes Feb 23, 2017

View reviewed changes

dfontenot force-pushed the cache_proxy_bypass branch from ab9fc8e to 2d4438d Compare February 23, 2017 22:03

wrapped proxy_bypass() with cache lookup

c121b98

Used to alleviate long gethostbyaddr calls Made new TimedCache and decorator to wrap a function with a cache * Entries looked up older than a minute (default amount) are evicted. * When full, evicts the oldest entry

dfontenot force-pushed the cache_proxy_bypass branch from 2d4438d to c121b98 Compare February 23, 2017 22:04

Lukasa approved these changes Feb 23, 2017

View reviewed changes

Lukasa merged commit 45a2977 into psf:master Feb 27, 2017

sigmavirus24 mentioned this pull request Mar 4, 2017

Proxy bypass detection could add delay of ~4.5 seconds to each request #2988

Closed

sigmavirus24 reviewed Mar 4, 2017

View reviewed changes

sigmavirus24 mentioned this pull request Mar 4, 2017

Update implementation of TimedCache object #3909

Merged

nateprewitt mentioned this pull request May 4, 2017

improve proxy bypass on Windows #3979

Merged

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

		class TestTimedCacheManagedDecorator(object):

		def test_caches_repeated_calls(self, mocker):

Cache proxy bypass #3885

Cache proxy bypass #3885

Conversation

dfontenot commented Feb 22, 2017

codecov-io commented Feb 22, 2017 • edited

Codecov Report

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfontenot commented Feb 22, 2017

Lukasa commented Feb 22, 2017

dfontenot commented Feb 22, 2017

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfontenot commented Feb 23, 2017

Lukasa commented Feb 23, 2017

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfontenot commented Feb 23, 2017

Lukasa left a comment

Choose a reason for hiding this comment

sigmavirus24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 22, 2017 •

edited