launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/librarian-client-retry into lp:launchpad.

Commit message:
Make the librarian client retry uploads in the event of an empty response from the server.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #907819 in Launchpad itself: "Oops on binary upload"
  https://bugs.launchpad.net/launchpad/+bug/907819

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/librarian-client-retry/+merge/224408

Occasionally the librarian server returns empty responses to the client, causing build upload failures that just say "Server said: ".  This seems to happen more with larger files: we have a >1% failure rate with livefs uploads so far, which is particularly annoying there since they're a bit more effort to retry.  The client could retry a couple of times to paper over transient problems.

QA: We don't have a reliable way to reproduce this on demand, so the test suite will have to do, but of course we should just check that ordinary build uploads still work.
-- 
https://code.launchpad.net/~cjwatson/launchpad/librarian-client-retry/+merge/224408
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/librarian-client-retry into lp:launchpad.

=== modified file 'lib/lp/services/librarian/client.py'
--- lib/lp/services/librarian/client.py	2013-06-20 05:50:00 +0000
+++ lib/lp/services/librarian/client.py	2014-06-25 10:11:50 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -13,6 +13,7 @@
     ]
 
 
+from functools import wraps
 import hashlib
 from select import select
 import socket
@@ -71,6 +72,27 @@
         return urljoin(base_url, alias_path)
 
 
+class UploadFailedRetry(UploadFailed):
+    """An upload failed, but it may be worth retrying."""
+
+
+def retry_failures(func):
+    """Decorator that retries on UploadFailedRetry, up to three times."""
+    @wraps(func)
+    def retry_func(*args, **kwargs):
+        for i in range(2):
+            try:
+                return func(*args, **kwargs)
+            except UploadFailedRetry:
+                continue
+        try:
+            return func(*args, **kwargs)
+        except UploadFailedRetry as e:
+            raise UploadFailed(*e.args)
+
+    return retry_func
+
+
 class FileUploadClient:
     """Simple blocking client for uploading to the librarian."""
 
@@ -98,10 +120,15 @@
         del self.state.s
         del self.state.f
 
+    def _fail(self, response):
+        if response:
+            raise UploadFailed('Server said: ' + response)
+        else:
+            raise UploadFailedRetry('Server returned an empty response')
+
     def _checkError(self):
         if select([self.state.s], [], [], 0)[0]:
-            response = self.state.f.readline().strip()
-            raise UploadFailed('Server said: ' + response)
+            self._fail(self.state.f.readline().strip())
 
     def _sendLine(self, line, check_for_error_responses=True):
         self.state.f.write(line + '\r\n')
@@ -111,6 +138,7 @@
     def _sendHeader(self, name, value):
         self._sendLine('%s: %s' % (name, value))
 
+    @retry_failures
     def addFile(self, name, size, file, contentType, expires=None,
                 debugID=None, allow_zero_length=False):
         """Add a file to the librarian.
@@ -201,7 +229,7 @@
             # Read response
             response = self.state.f.readline().strip()
             if response != '200':
-                raise UploadFailed('Server said: ' + response)
+                self._fail(response)
 
             # Add rows to DB
             content = LibraryFileContent(

=== modified file 'lib/lp/services/librarian/tests/test_client.py'
--- lib/lp/services/librarian/tests/test_client.py	2014-01-30 15:04:06 +0000
+++ lib/lp/services/librarian/tests/test_client.py	2014-06-25 10:11:50 +0000
@@ -1,11 +1,10 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from cStringIO import StringIO
 import hashlib
 import httplib
 import textwrap
-import unittest
 from urllib2 import (
     HTTPError,
     URLError,
@@ -25,6 +24,7 @@
     )
 from lp.services.librarian.interfaces.client import UploadFailed
 from lp.services.librarian.model import LibraryFileAlias
+from lp.testing import TestCase
 from lp.testing.layers import (
     DatabaseLayer,
     FunctionalLayer,
@@ -35,8 +35,9 @@
 
 class InstrumentedLibrarianClient(LibrarianClient):
 
-    def __init__(self, *args, **kwargs):
+    def __init__(self, max_errors=0, *args, **kwargs):
         super(InstrumentedLibrarianClient, self).__init__(*args, **kwargs)
+        self.max_errors = max_errors
         self.check_error_calls = 0
 
     sentDatabaseName = False
@@ -54,7 +55,10 @@
 
     def _checkError(self):
         self.check_error_calls += 1
-        super(InstrumentedLibrarianClient, self)._checkError()
+        if self.check_error_calls <= self.max_errors:
+            self._fail('')
+        else:
+            super(InstrumentedLibrarianClient, self)._checkError()
 
 
 def make_mock_file(error, max_raise):
@@ -79,7 +83,7 @@
     return mock_file
 
 
-class LibrarianClientTestCase(unittest.TestCase):
+class LibrarianClientTestCase(TestCase):
     layer = LaunchpadFunctionalLayer
 
     def test_addFileSendsDatabaseName(self):
@@ -173,6 +177,27 @@
         self.assertEqual(sha1, lfa.content.sha1)
         self.assertEqual(sha256, lfa.content.sha256)
 
+    def test_addFile_error_retry(self):
+        # Occasionally the librarian server returns empty responses (for
+        # unclear reasons).  The client retries a few times in this case.
+        client = InstrumentedLibrarianClient(max_errors=1)
+        alias_id = client.addFile(
+            'sample.txt', 6, StringIO('sample'), 'text/plain')
+        # On the first try, addFile() calls _sendLine() once.  On the
+        # second, it calls _sendHeader() three times and _sendLine() twice.
+        self.assertEqual(6, client.check_error_calls)
+        # The file made it in the end.
+        transaction.commit()
+        self.assertEqual('sample', client.getFileByAlias(alias_id).read())
+
+    def test_addFile_error_max_retries(self):
+        # The client gives up if the librarian server fails with empty
+        # responses too many times.
+        client = InstrumentedLibrarianClient(max_errors=3)
+        self.assertRaisesWithContent(
+            UploadFailed, 'Server returned an empty response',
+            client.addFile, 'sample.txt', 6, StringIO('sample'), 'text/plain')
+
     def test__getURLForDownload(self):
         # This protected method is used by getFileByAlias. It is supposed to
         # use the internal host and port rather than the external, proxied
@@ -330,7 +355,7 @@
         client_module._File = _File
 
 
-class TestWebServiceErrors(unittest.TestCase):
+class TestWebServiceErrors(TestCase):
     """ Test that errors are correctly mapped to HTTP status codes."""
 
     layer = FunctionalLayer