launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-750274 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-750274/+merge/63129

I hope that this branch fixes bug 750274: librarian.txt fails sometimes. I find it hard to be sure that a patch really fixes all possible causes for such a low-level communication problem, but I think my suspicion about the cause is reasonable:

Both tracebacks mentioned in the bug report show that the error occurs when an empty file is uploaded. In other words: The client sends only a few header lines and nothing more. After sending these header lines, the server is supposed to return a success response -- and it does so.

The problem is the error check in FileUploadClient._sendLine(): When _sendLine() has sent the empty line signalling the end of the header, it calls _checkErrror(), which treats _any_ response from the server as an error.

If the server responds quick enough, the select() call in _checkError() succeeds, and the method raises UploadFailed(). I suspect that this happens when the kernel's process manager switches from the client process to the server process after "self.state.f.write(line + '\r\n')" in _sendLine() but before the select() call in _checkError().

This is a very small time window, so the error occurs only quite seldom.

The fix is obvious: the sendLine() call for the empty line signalling "all headers sent" should not check if the server responded, if an empty file is uploaded. addFile() does the final check of the server's response a few lines below the the last _sendLine() call.

test: ./bin/test canonical -vvt test_client

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-750274/+merge/63129
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-750274 into lp:launchpad.

=== modified file 'lib/canonical/librarian/client.py'
--- lib/canonical/librarian/client.py	2011-01-27 09:19:44 +0000
+++ lib/canonical/librarian/client.py	2011-06-01 15:23:36 +0000
@@ -95,9 +95,10 @@
             response = self.state.f.readline().strip()
             raise UploadFailed('Server said: ' + response)
 
-    def _sendLine(self, line):
+    def _sendLine(self, line, check_for_error_responses=True):
         self.state.f.write(line + '\r\n')
-        self._checkError()
+        if check_for_error_responses:
+            self._checkError()
 
     def _sendHeader(self, name, value):
         self._sendLine('%s: %s' % (name, value))
@@ -162,8 +163,11 @@
             if debugID is not None:
                 self._sendHeader('Debug-ID', debugID)
 
-            # Send blank line
-            self._sendLine('')
+            # Send blank line. Do not check for a response from the
+            # server when no data will be sent. Otherwise
+            # _checkError() might consume the "200" response which
+            # is supposed to be read below in this method.
+            self._sendLine('', check_for_error_responses=(size > 0))
 
             # Prepare to the upload the file
             shaDigester = hashlib.sha1()
@@ -173,7 +177,7 @@
             # Read in and upload the file 64kb at a time, by using the two-arg
             # form of iter (see
             # /usr/share/doc/python2.4/html/lib/built-in-funcs.html#l2h-42).
-            for chunk in iter(lambda: file.read(1024*64), ''):
+            for chunk in iter(lambda: file.read(1024 * 64), ''):
                 self.state.f.write(chunk)
                 bytesWritten += len(chunk)
                 shaDigester.update(chunk)
@@ -241,7 +245,7 @@
             # Read in and upload the file 64kb at a time, by using the two-arg
             # form of iter (see
             # /usr/share/doc/python2.4/html/lib/built-in-funcs.html#l2h-42).
-            for chunk in iter(lambda: file.read(1024*64), ''):
+            for chunk in iter(lambda: file.read(1024 * 64), ''):
                 self.state.f.write(chunk)
                 bytesWritten += len(chunk)
 

=== modified file 'lib/canonical/librarian/ftests/test_client.py'
--- lib/canonical/librarian/ftests/test_client.py	2011-01-26 23:54:06 +0000
+++ lib/canonical/librarian/ftests/test_client.py	2011-06-01 15:23:36 +0000
@@ -21,6 +21,11 @@
 
 
 class InstrumentedLibrarianClient(LibrarianClient):
+
+    def __init__(self, *args, **kwargs):
+        super(InstrumentedLibrarianClient, self).__init__(*args, **kwargs)
+        self.check_error_calls = 0
+
     sentDatabaseName = False
     def _sendHeader(self, name, value):
         if name == 'Database-Name':
@@ -32,6 +37,10 @@
         self.called_getURLForDownload = True
         return LibrarianClient._getURLForDownload(self, aliasID)
 
+    def _checkError(self):
+        self.check_error_calls += 1
+        super(InstrumentedLibrarianClient, self)._checkError()
+
 
 def make_mock_file(error, max_raise):
     """Return a surrogate for client._File.
@@ -108,6 +117,31 @@
         f = client.getFileByAlias(alias_id)
         self.assertEqual(f.read(), 'sample')
 
+    def test_addFile_no_response_check_at_end_headers_for_empty_file(self):
+        # When addFile() sends the request header, it checks if the
+        # server responded with an error response after sending each
+        # header line. It does _not_ do this check when it sends the
+        # empty line following the headers.
+        client = InstrumentedLibrarianClient()
+        client.addFile(
+            'sample.txt', 0, StringIO(''), 'text/plain',
+            allow_zero_length=True)
+        # addFile() calls _sendHeader() three times and _sendLine()
+        # twice, but it does not check if the server responded
+        # in the second call.
+        self.assertEqual(4, client.check_error_calls)
+
+    def test_addFile_response_check_at_end_headers_for_non_empty_file(self):
+        # When addFile() sends the request header, it checks if the
+        # server responded with an error response after sending each
+        # header line. It does _not_ do this check when it sends the
+        # empty line following the headers.
+        client = InstrumentedLibrarianClient()
+        client.addFile('sample.txt', 4, StringIO('1234'), 'text/plain')
+        # addFile() calls _sendHeader() three times and _sendLine()
+        # twice.
+        self.assertEqual(5, client.check_error_calls)
+
     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