Merge ~laney/germinate:urlopen-timeout into germinate:master

[Iain Lane]

We're seeing this happening increasingly frequently in livefs builds now. I'm not sure what changed. Here's a first cut at this. Couple of thoughts

  - Are the constants OK?
  - Should I back off?

If the bug comes back then the new test will hang forever. Not sure if I should add a timeout there, or how to do that really.

[Colin Watson (cjwatson)]

We should probably switch the HTTP library here to requests at some point, but something like this will do for now.

review: Needs Fixing

[Iain Lane (laney)]

Cheers, I've fixed it as requested.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/germinate/seeds.py b/germinate/seeds.py
index 98ac34f..e3cfdaa 100644
--- a/germinate/seeds.py
+++ b/germinate/seeds.py
@@ -28,6 +28,7 @@ import logging
 import os
 import re
 import shutil
+import socket
 import subprocess
 import sys
 import tempfile
@@ -107,6 +108,8 @@ class SeedVcs(object):
 
 class Seed(six.Iterator):
     """A single seed from a collection."""
+    _n_retries = 5  # how many times to try downloading a URL
+    _retry_timeout = 10  # timeout between each of the above retries
 
     def _open_seed_bzr(self, base, branch, name):
         global _vcs_cache_dir
@@ -177,11 +180,20 @@ class Seed(six.Iterator):
             fullpath = os.path.join(path, name)
             _logger.info("Using %s", fullpath)
             return open(fullpath)
-        _logger.info("Downloading %s", url)
         req = Request(url)
         req.add_header('Cache-Control', 'no-cache')
         req.add_header('Pragma', 'no-cache')
-        return urlopen(req)
+        for n in reversed(range(Seed._n_retries)):  # try 5 times
+            try:
+                _logger.info("Downloading %s", url)
+                return urlopen(req, timeout=Seed._retry_timeout)
+            except socket.timeout as e:
+                err = e
+                if n > 0:
+                    _logger.warning("Timed out opening %s, retrying...", url)
+
+        _logger.error("Timed out opening %s, failing.", url)
+        raise err
 
     def _open_seed(self, base, branch, name, vcs=None):
         if vcs is not None:
diff --git a/germinate/tests/test_seeds.py b/germinate/tests/test_seeds.py
index f747b0a..b92ec90 100644
--- a/germinate/tests/test_seeds.py
+++ b/germinate/tests/test_seeds.py
@@ -17,13 +17,20 @@
 # Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
 # 02110-1301, USA.
 
+import errno
 import io
 import os
+import random
+import socket
 import textwrap
+import threading
+
+from fixtures import MockPatch
 
 from germinate.seeds import (
     AtomicFile,
     Seed,
+    SeedError,
     SingleSeedStructure,
     )
 from germinate.tests.helpers import TestCase
@@ -109,6 +116,64 @@ class TestSeed(TestCase):
             self.assertTrue(" * foo\n", lines[0])
 
 
+class TestSeedHTTP(TestCase):
+    def setUp(self):
+        """ Make a server which reads anything you send it but never responds
+        to you, to test timeouts are triggered properly. """
+        super(TestSeedHTTP, self).setUp()
+        self.n_hits = 0
+        self.server = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        while True:
+            try:
+                self.port = random.randint(1025, 65535)
+                self.server.bind(("localhost", self.port))
+                break
+            except OSError as e:
+                if e.errno != errno.EADDRINUSE:
+                    raise
+        self.server.listen(1)
+
+        def accept():
+            while self.server is not None:
+                try:
+                    (client, _) = self.server.accept()
+                    self.n_hits += 1
+                except (OSError, socket.error) as e:
+                    if e.errno in (errno.EBADF, errno.EINVAL):  # closed
+                        return
+                    raise
+                # We'll read the HTTP request, not reply and the caller should
+                # eventually timeout
+                while True:
+                    if not client.recv(1024):
+                        break
+                client.close()
+
+        self.thread = threading.Thread(target=accept)
+        self.thread.start()
+
+    def tearDown(self):
+        self.server.shutdown(socket.SHUT_RDWR)
+        self.server.close()
+        self.server = None
+        self.thread.join()
+        super(TestSeedHTTP, self).tearDown()
+
+    def test_init_http_timeout(self):
+        """ When we connect to a server which doesn't respond, we timeout. """
+        fixture = MockPatch("germinate.seeds.Seed._retry_timeout", 0.1)
+        with fixture:
+            try:
+                Seed(
+                    ["http://localhost:%s" % self.port],
+                    ["collection.dist"],
+                    "test")
+            except SeedError:
+                self.assertEqual(self.n_hits, 5)
+                return
+            self.assertFalse(True, "SeedError was not raised")
+
+
 class TestSingleSeedStructure(TestCase):
     def test_basic(self):
         """A SingleSeedStructure object has the correct basic properties."""