launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Stuart Bishop has proposed merging lp:~stub/launchpad/trivial into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #951401 in Launchpad itself: "parse-ppa-apache-logs failing (missing files)"
  https://bugs.launchpad.net/launchpad/+bug/951401
  Bug #1263002 in Launchpad itself: "Twisted feature flag support fails typecasting when updating"
  https://bugs.launchpad.net/launchpad/+bug/1263002

For more details, see:
https://code.launchpad.net/~stub/launchpad/trivial/+merge/245822

Update to using python-swiftclient 2.3.1.

We now can call the close() method explicitly, although at first glance this appears to be a noop with the default HTTPConnection implementation. We still hope this fixes leaks though, as the new library also switches to using the requests library.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/launchpad/trivial into lp:launchpad.

=== modified file 'lib/lp/services/librarianserver/storage.py'
--- lib/lp/services/librarianserver/storage.py	2014-10-17 09:21:23 +0000
+++ lib/lp/services/librarianserver/storage.py	2015-01-08 08:50:52 +0000
@@ -100,9 +100,11 @@
             defer.returnValue(swift_stream)
         except swiftclient.ClientException as x:
             if x.http_status != 404:
+                swift_connection.close()
                 self.swift_download_fails += 1
                 log.err(x)
         except Exception as x:
+            swift_connection.close()
             self.swift_download_fails += 1
             log.err(x)
 

=== modified file 'lib/lp/services/librarianserver/swift.py'
--- lib/lp/services/librarianserver/swift.py	2014-12-16 09:20:15 +0000
+++ lib/lp/services/librarianserver/swift.py	2015-01-08 08:50:52 +0000
@@ -122,6 +122,7 @@
                 log.debug2('{0} container already exists'.format(container))
             except swiftclient.ClientException as x:
                 if x.http_status != 404:
+                    swift_connection.close()
                     raise
                 log.info('Creating {0} container'.format(container))
                 swift_connection.put_container(container)
@@ -138,6 +139,7 @@
                         '{0} has incorrect size in Swift'.format(lfc))
             except swiftclient.ClientException as x:
                 if x.http_status != 404:
+                    swift_connection.close()
                     raise
                 log.info('Putting {0} into Swift ({1}, {2})'.format(
                     lfc, container, obj_name))
@@ -189,8 +191,13 @@
             seg_name = '%s/%04d' % (obj_name, segment)
             seg_size = min(fs_size - fs_file.tell(), MAX_SWIFT_OBJECT_SIZE)
             md5_stream = HashStream(fs_file)
+            start_position = md5_stream.tell()
             swift_md5_hash = swift_connection.put_object(
-                container, seg_name, md5_stream, seg_size)
+                container, seg_name,
+                LimitedStream(md5_stream, seg_size), seg_size)
+            end_position = md5_stream.tell()
+            assert end_position - start_position == seg_size, 'Read {}'.format(
+                end_position - start_position)
             segment_md5_hash = md5_stream.hash.hexdigest()
             assert swift_md5_hash == segment_md5_hash, (
                 "LibraryFileContent({0}) segment {1} upload corrupted".format(
@@ -290,6 +297,7 @@
 
     def close(self):
         self.closed = True
+        self._swift_connection.close()
         self._swift_connection = None
 
     def seek(self, offset):
@@ -323,6 +331,31 @@
         return self._stream.seek(offset)
 
 
+class LimitedStream:
+    """Limit the amount of data that can be read from a stream.
+
+    It seems the underlying requests library reads in fixed sized chunks,
+    rather than honoring the content length and only reading what it needs,
+    so we need to enforce the segment length cuttoff ourselves. If more data
+    is read than necessary from a HashStream, then it will calculate an
+    unexpected hash.
+    """
+    def __init__(self, stream, content_length):
+        self._stream = stream
+        self._content_length = content_length
+        self._bytes_read = 0
+
+    def read(self, size=-1):
+        remaining = self._content_length - self._bytes_read
+        if size == -1:
+            size = remaining
+        size = min(size, remaining)
+        chunk = self._stream.read(size)
+        self._bytes_read += len(chunk)
+        assert self._bytes_read <= self._content_length
+        return chunk
+
+
 class ConnectionPool:
     MAX_POOL_SIZE = 10
 
@@ -349,7 +382,8 @@
         if swift_connection not in self._pool:
             self._pool.append(swift_connection)
             while len(self._pool) > self.MAX_POOL_SIZE:
-                self._pool.pop(0)
+                c = self._pool.pop(0)
+                c.close()
 
     def _new_connection(self):
         return swiftclient.Connection(

=== modified file 'lib/lp/testing/swift/tests/test_fixture.py'
--- lib/lp/testing/swift/tests/test_fixture.py	2013-12-12 08:17:53 +0000
+++ lib/lp/testing/swift/tests/test_fixture.py	2015-01-08 08:50:52 +0000
@@ -8,6 +8,7 @@
 
 import httplib
 
+import requests.exceptions
 from s4 import hollow
 from swiftclient import client as swiftclient
 
@@ -61,14 +62,14 @@
         # authenticated.
         self.swift_fixture.shutdown()
         self.assertRaises(
-            httplib.HTTPException,
+            requests.exceptions.ConnectionError,
             client.get_object, "size", str(size))
 
         # And even if we bring it back up, existing connections
         # continue to fail
         self.swift_fixture.startup()
         self.assertRaises(
-            httplib.HTTPException,
+            swiftclient.ClientException,
             client.get_object, "size", str(size))
 
         # But fresh connections are fine.

=== modified file 'lib/lp_sitecustomize.py'
--- lib/lp_sitecustomize.py	2014-08-29 05:52:23 +0000
+++ lib/lp_sitecustomize.py	2015-01-08 08:50:52 +0000
@@ -88,14 +88,9 @@
 
 
 def silence_swiftclient_logger():
-    """Remove debug noise generated by swiftclient and keystoneclient.
-
-    swiftclient explicitly checks if debug messages are enabled on its
-    Logger, which is unfortunate as we disable them in the handler. Not
-    only does swiftclient then emit lots of noise, but it also turns
-    keystoneclient debugging on.
-    """
-    swiftclient.logger.setLevel(logging.INFO)
+    """Install the NullHandler on the swiftclient logger to silence noise."""
+    swiftclient.logger.addHandler(logging.NullHandler())
+    swiftclient.logger.propagate = False
 
 
 def silence_zcml_logger():

=== modified file 'versions.cfg'
--- versions.cfg	2014-12-08 02:23:41 +0000
+++ versions.cfg	2015-01-08 08:50:52 +0000
@@ -33,6 +33,8 @@
 feedvalidator = 0.0.0DEV-r1049
 fixtures = 0.3.9
 funkload = 1.16.1
+# Required by six
+futures = 2.2.0
 grokcore.component = 1.6
 html5browser = 0.0.9
 httplib2 = 0.8
@@ -102,7 +104,7 @@
 # reapplied a patch from wgrant to get codehosting going again.
 python-openid = 2.2.5-fix1034376
 python-subunit = 0.0.8beta
-python-swiftclient = 1.5.0
+python-swiftclient = 2.3.1
 rabbitfixture = 0.3.5
 requests = 1.2.3
 s4 = 0.1.2
@@ -110,7 +112,7 @@
 setuptools-git = 1.0
 simplejson = 3.1.3
 SimpleTAL = 4.3
-six = 1.3.0
+six = 1.9.0
 soupmatchers = 0.2
 sourcecodegen = 0.6.14
 # lp:~launchpad-committers/storm/with-without-datetime