Merge ~adam-collard/simplestreams:timeout-mirror into simplestreams:master

[Adam Collard]

Add 10s timeout to out-going requests to avoid blocking.

Fixes LP:1908452

Suggestions on how to best test this are welcome

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:b09f3a2230be425c588a0cb3288476f35eb17ef3
https://jenkins.ubuntu.com/server/job/simplestreams-ci/4/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/simplestreams-ci/nodes=metal-amd64/4/
    SUCCESS: https://jenkins.ubuntu.com/server/job/simplestreams-ci/nodes=metal-ppc64el/4/
    SUCCESS: https://jenkins.ubuntu.com/server/job/simplestreams-ci/nodes=metal-s390x/4/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/simplestreams-ci/4//rebuild

review: Approve (continuous-integration)

[dann frazier (dannf)]

I do wonder if it makes sense for python3-simplestreams to be instituting "One True Timeout" value for every caller, since I imagine that is application/environment-dependent. IMHO, it seems like simplestreams could expose a new timeout parameter (possibly with a non-zero default) in it's class init() methods that callers could override.

For MAAS, for example, I think a 10s timeout could still result in noticeable UI latency issues (every 4th regiond API call could hang for up to 10s during a sync), so it might want to select something smaller. However, command line tools like those included in simplestreams would likely prefer to be more patient.

[dann frazier (dannf)]

Oh, and regarding testing, here's the best reproducer I've come up with:
  https://bugs.launchpad.net/maas/+bug/1908452/comments/19

Unfortunately it's not a great option for an automated test case. Perhaps using a python-based https server w/ an intentionally broken handshake would work?

[Paride Legovini (paride)]

LGTM. It is not easy to test this in a way that is meaningful for simplestreams, and this MP is a plain implementation of what's recommended by requests for production code ([1], as ltrager pointed out). I think we can land this.

[1] https://requests.readthedocs.io/en/master/user/quickstart/#timeouts

review: Approve

diff --git a/simplestreams/contentsource.py b/simplestreams/contentsource.py
index 40bf9d0..b54e898 100644
--- a/simplestreams/contentsource.py
+++ b/simplestreams/contentsource.py
@@ -32,6 +32,7 @@ else:
     urllib_error = urllib_request
 
 READ_BUFFER_SIZE = 1024 * 10
+TIMEOUT = 10
 
 try:
     # We try to use requests because we can do gzip encoding with it.
@@ -338,7 +339,7 @@ class Urllib2UrlReader(UrlReader):
                 req.add_header('User-Agent', user_agent)
             if offset is not None:
                 req.add_header('Range', 'bytes=%d-' % offset)
-            self.req = opener(req)
+            self.req = opener(req, timeout=TIMEOUT)
         except urllib_error.HTTPError as e:
             if e.code == 404:
                 myerr = IOError("Unable to open %s" % url)
@@ -378,7 +379,9 @@ class RequestsUrlReader(UrlReader):
         if headers == {}:
             headers = None
 
-        self.req = requests.get(url, stream=True, auth=auth, headers=headers)
+        self.req = requests.get(
+            url, stream=True, auth=auth, headers=headers, timeout=TIMEOUT
+        )
         self.r_iter = None
         if buflen is None:
             buflen = READ_BUFFER_SIZE