Merge ~adam-collard/simplestreams:remove-requests-read-timeout into simplestreams:master

[Adam Collard]

Remove requests read timeout, whilst retaining connect timeout

Drive-by: make the negative tests actually test negative sizes

Fix for bug introduced in f37d2ed - the intention there was only
for a connect timeout and not a read timeout.

Changed the timeout to just over a multiple of 3s to match advice in https://docs.python-requests.org/en/master/user/advanced/#timeouts

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

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

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

review: Approve (continuous-integration)

[Paride Legovini (paride)]

LGTM, thanks for the pointer on why 9.05s, TIL!

review: Approve

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

Commit message lints:
- Line #4 has 36 too many characters. Line starts with: "Fix for bug introduced"...

review: Needs Fixing

[Paride Legovini (paride)]

Rewrapped

review: Approve

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

review: Approve (continuous-integration)

diff --git a/simplestreams/contentsource.py b/simplestreams/contentsource.py
index b54e898..55028a7 100644
--- a/simplestreams/contentsource.py
+++ b/simplestreams/contentsource.py
@@ -32,7 +32,7 @@ else:
     urllib_error = urllib_request
 
 READ_BUFFER_SIZE = 1024 * 10
-TIMEOUT = 10
+TIMEOUT = 9.05
 
 try:
     # We try to use requests because we can do gzip encoding with it.
@@ -322,6 +322,9 @@ class FileReader(UrlReader):
 
 
 class Urllib2UrlReader(UrlReader):
+
+    timeout = TIMEOUT
+
     def __init__(self, url, offset=None, user_agent=None):
         (url, username, password) = parse_url_auth(url)
         self.url = url
@@ -339,7 +342,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, timeout=TIMEOUT)
+            self.req = opener(req, timeout=self.timeout)
         except urllib_error.HTTPError as e:
             if e.code == 404:
                 myerr = IOError("Unable to open %s" % url)
@@ -355,11 +358,18 @@ class Urllib2UrlReader(UrlReader):
 
 
 class RequestsUrlReader(UrlReader):
-    # This provides a url reader that supports deflate/gzip encoding
-    # but still implements 'read'.
-    # r = RequestsUrlReader(http://example.com)
-    # r.read(10)
-    # r.close()
+    """
+    This provides a url reader that supports deflate/gzip encoding
+    but still implements 'read'.
+    r = RequestsUrlReader('http://example.com')
+    r.read(10)
+    r.close()
+    """
+
+    # requests takes a 2-tuple for timeout, the first being a
+    # connect timeout, the second being a read timeout.
+    timeout = (TIMEOUT, None)
+
     def __init__(self, url, buflen=None, offset=None, user_agent=None):
         if requests is None:
             raise ImportError("Attempt to use RequestsUrlReader "
@@ -380,7 +390,7 @@ class RequestsUrlReader(UrlReader):
             headers = None
 
         self.req = requests.get(
-            url, stream=True, auth=auth, headers=headers, timeout=TIMEOUT
+            url, stream=True, auth=auth, headers=headers, timeout=self.timeout
         )
         self.r_iter = None
         if buflen is None:
diff --git a/tests/unittests/test_contentsource.py b/tests/unittests/test_contentsource.py
index 6487494..77a566d 100644
--- a/tests/unittests/test_contentsource.py
+++ b/tests/unittests/test_contentsource.py
@@ -170,6 +170,11 @@ class TestUrlContentSource(BaseDirUsingTestCase):
         self.assertEqual(data, self.fdata)
 
     @skipIf(contentsource.requests is None, "requests not available")
+    def test_requests_default_timeout(self):
+        self.assertEqual(contentsource.RequestsUrlReader.timeout,
+                         (contentsource.TIMEOUT, None))
+
+    @skipIf(contentsource.requests is None, "requests not available")
     def test_requests_url_read_handles_None(self):
         scs = self.getcs(self.fpath, contentsource.RequestsUrlReader)
         data = scs.read(None)
@@ -178,7 +183,7 @@ class TestUrlContentSource(BaseDirUsingTestCase):
     @skipIf(contentsource.requests is None, "requests not available")
     def test_requests_url_read_handles_negative_size(self):
         scs = self.getcs(self.fpath, contentsource.RequestsUrlReader)
-        data = scs.read(None)
+        data = scs.read(-2)
         self.assertEqual(data, self.fdata)
 
     @skipIf(contentsource.requests is None, "requests not available")
@@ -193,6 +198,10 @@ class TestUrlContentSource(BaseDirUsingTestCase):
         data = scs.read(3)
         self.assertEqual(data, self.fdata[0:3])
 
+    def test_urllib_default_timeout(self):
+        self.assertEqual(contentsource.Urllib2UrlReader.timeout,
+                         contentsource.TIMEOUT)
+
     def test_urllib_url_read_handles_None(self):
         scs = self.getcs(self.fpath, contentsource.Urllib2UrlReader)
         data = scs.read(None)
@@ -200,7 +209,7 @@ class TestUrlContentSource(BaseDirUsingTestCase):
 
     def test_urllib_url_read_handles_negative_size(self):
         scs = self.getcs(self.fpath, contentsource.Urllib2UrlReader)
-        data = scs.read(None)
+        data = scs.read(-2)
         self.assertEqual(data, self.fdata)
 
     def test_urllib_url_read_handles_no_size(self):