Launchpad itself

Merge lp:~stub/launchpad/trivial into lp:launchpad

  1. trivial
  2. Merge into devel
Proposed by Stuart Bishop
Status: Rejected
Rejected by: William Grant
Proposed branch: lp:~stub/launchpad/trivial
Merge into: lp:launchpad
Diff against target: 224 lines (+80/-19)
5 files modified
lib/lp/services/librarianserver/storage.py (+5/-1)
lib/lp/services/librarianserver/swift.py (+64/-5)
lib/lp/testing/swift/tests/test_fixture.py (+3/-2)
lib/lp_sitecustomize.py (+3/-8)
versions.cfg (+5/-3)
To merge this branch: bzr merge lp:~stub/launchpad/trivial
Reviewer Review Type Date Requested Status
William Grant Disapprove
Review via email: mp+245822@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
lp:~stub/launchpad/trivial updated
10089. By Stuart Bishop

requests 2.5.1

10090. By Stuart Bishop

Close potential HTTP connection leaks, working around Bug #1408616

Revision history for this message
Stuart Bishop (stub) wrote :

Updated things so close() is not a noop.

lp:~stub/launchpad/trivial updated
10091. By Stuart Bishop

Return swift connections to the pool on 404

Revision history for this message
William Grant (wgrant) wrote :

We seem to be fine without this now, and the swiftclient upgrade isn't quite that easy due to setup_requires.

review: Disapprove

Unmerged revisions

10091. By Stuart Bishop

Return swift connections to the pool on 404

10090. By Stuart Bishop

Close potential HTTP connection leaks, working around Bug #1408616

10089. By Stuart Bishop

requests 2.5.1

10088. By Stuart Bishop

Swift fixes for swiftclient 2.3.1

10087. By Stuart Bishop

Fix swift fixture tests

10086. By Stuart Bishop

python-swiftclient 2.3.1

10085. By Stuart Bishop

Merge trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/librarianserver/storage.py'
2--- lib/lp/services/librarianserver/storage.py 2014-10-17 09:21:23 +0000
3+++ lib/lp/services/librarianserver/storage.py 2015-01-09 05:17:45 +0000
4@@ -99,10 +99,14 @@
5 swift_stream = TxSwiftStream(swift_connection, chunks)
6 defer.returnValue(swift_stream)
7 except swiftclient.ClientException as x:
8- if x.http_status != 404:
9+ if x.http_status == 404:
10+ swift.connection_pool.put(swift_connection)
11+ else:
12+ swift_connection.close()
13 self.swift_download_fails += 1
14 log.err(x)
15 except Exception as x:
16+ swift_connection.close()
17 self.swift_download_fails += 1
18 log.err(x)
19
20
21=== modified file 'lib/lp/services/librarianserver/swift.py'
22--- lib/lp/services/librarianserver/swift.py 2014-12-16 09:20:15 +0000
23+++ lib/lp/services/librarianserver/swift.py 2015-01-09 05:17:45 +0000
24@@ -122,6 +122,7 @@
25 log.debug2('{0} container already exists'.format(container))
26 except swiftclient.ClientException as x:
27 if x.http_status != 404:
28+ swift_connection.close()
29 raise
30 log.info('Creating {0} container'.format(container))
31 swift_connection.put_container(container)
32@@ -138,6 +139,7 @@
33 '{0} has incorrect size in Swift'.format(lfc))
34 except swiftclient.ClientException as x:
35 if x.http_status != 404:
36+ swift_connection.close()
37 raise
38 log.info('Putting {0} into Swift ({1}, {2})'.format(
39 lfc, container, obj_name))
40@@ -189,8 +191,13 @@
41 seg_name = '%s/%04d' % (obj_name, segment)
42 seg_size = min(fs_size - fs_file.tell(), MAX_SWIFT_OBJECT_SIZE)
43 md5_stream = HashStream(fs_file)
44+ start_position = md5_stream.tell()
45 swift_md5_hash = swift_connection.put_object(
46- container, seg_name, md5_stream, seg_size)
47+ container, seg_name,
48+ LimitedStream(md5_stream, seg_size), seg_size)
49+ end_position = md5_stream.tell()
50+ assert end_position - start_position == seg_size, 'Read {}'.format(
51+ end_position - start_position)
52 segment_md5_hash = md5_stream.hash.hexdigest()
53 assert swift_md5_hash == segment_md5_hash, (
54 "LibraryFileContent({0}) segment {1} upload corrupted".format(
55@@ -290,6 +297,7 @@
56
57 def close(self):
58 self.closed = True
59+ self._swift_connection.close()
60 self._swift_connection = None
61
62 def seek(self, offset):
63@@ -323,6 +331,57 @@
64 return self._stream.seek(offset)
65
66
67+class LimitedStream:
68+ """Limit the amount of data that can be read from a stream.
69+
70+ It seems the underlying requests library reads in fixed sized chunks,
71+ rather than honoring the content length and only reading what it needs,
72+ so we need to enforce the segment length cuttoff ourselves. If more data
73+ is read than necessary from a HashStream, then it will calculate an
74+ unexpected hash.
75+ """
76+ def __init__(self, stream, content_length):
77+ self._stream = stream
78+ self._content_length = content_length
79+ self._bytes_read = 0
80+
81+ def read(self, size=-1):
82+ remaining = self._content_length - self._bytes_read
83+ if size == -1:
84+ size = remaining
85+ size = min(size, remaining)
86+ chunk = self._stream.read(size)
87+ self._bytes_read += len(chunk)
88+ assert self._bytes_read <= self._content_length
89+ return chunk
90+
91+
92+class HTTPConnectionWithClose(swiftclient.HTTPConnection):
93+ '''A swiftclient.HTTPConnection that actually defines a close() method.
94+
95+ This is a work around for Bug #1408616.
96+ '''
97+ def close(self):
98+ if hasattr(swiftclient.HTTPConnection, 'close'):
99+ # If swiftclient.HTTPConnection grows a close method, call it.
100+ super(HTTPConnection, self).close()
101+ self.requests_session.close()
102+
103+
104+class SwiftConnection(swiftclient.Connection):
105+ '''A swiftclient.Connection that uses a HTTPConnection that can
106+ be explicitly closed.
107+
108+ This is a work around for Bug #1408616.
109+ '''
110+ def http_connection(self):
111+ conn = HTTPConnectionWithClose(self.url,
112+ cacert=self.cacert,
113+ insecure=self.insecure,
114+ ssl_compression=self.ssl_compression)
115+ return conn.parsed_url, conn
116+
117+
118 class ConnectionPool:
119 MAX_POOL_SIZE = 10
120
121@@ -349,16 +408,16 @@
122 if swift_connection not in self._pool:
123 self._pool.append(swift_connection)
124 while len(self._pool) > self.MAX_POOL_SIZE:
125- self._pool.pop(0)
126+ c = self._pool.pop(0)
127+ c.close()
128
129 def _new_connection(self):
130- return swiftclient.Connection(
131+ return SwiftConnection(
132 authurl=config.librarian_server.os_auth_url,
133 user=config.librarian_server.os_username,
134 key=config.librarian_server.os_password,
135 tenant_name=config.librarian_server.os_tenant_name,
136- auth_version='2.0',
137- )
138+ auth_version='2.0')
139
140
141 connection_pool = ConnectionPool()
142
143=== modified file 'lib/lp/testing/swift/tests/test_fixture.py'
144--- lib/lp/testing/swift/tests/test_fixture.py 2013-12-12 08:17:53 +0000
145+++ lib/lp/testing/swift/tests/test_fixture.py 2015-01-09 05:17:45 +0000
146@@ -8,6 +8,7 @@
147
148 import httplib
149
150+import requests.exceptions
151 from s4 import hollow
152 from swiftclient import client as swiftclient
153
154@@ -61,14 +62,14 @@
155 # authenticated.
156 self.swift_fixture.shutdown()
157 self.assertRaises(
158- httplib.HTTPException,
159+ requests.exceptions.ConnectionError,
160 client.get_object, "size", str(size))
161
162 # And even if we bring it back up, existing connections
163 # continue to fail
164 self.swift_fixture.startup()
165 self.assertRaises(
166- httplib.HTTPException,
167+ swiftclient.ClientException,
168 client.get_object, "size", str(size))
169
170 # But fresh connections are fine.
171
172=== modified file 'lib/lp_sitecustomize.py'
173--- lib/lp_sitecustomize.py 2014-08-29 05:52:23 +0000
174+++ lib/lp_sitecustomize.py 2015-01-09 05:17:45 +0000
175@@ -88,14 +88,9 @@
176
177
178 def silence_swiftclient_logger():
179- """Remove debug noise generated by swiftclient and keystoneclient.
180-
181- swiftclient explicitly checks if debug messages are enabled on its
182- Logger, which is unfortunate as we disable them in the handler. Not
183- only does swiftclient then emit lots of noise, but it also turns
184- keystoneclient debugging on.
185- """
186- swiftclient.logger.setLevel(logging.INFO)
187+ """Install the NullHandler on the swiftclient logger to silence noise."""
188+ swiftclient.logger.addHandler(logging.NullHandler())
189+ swiftclient.logger.propagate = False
190
191
192 def silence_zcml_logger():
193
194=== modified file 'versions.cfg'
195--- versions.cfg 2014-12-08 02:23:41 +0000
196+++ versions.cfg 2015-01-09 05:17:45 +0000
197@@ -33,6 +33,8 @@
198 feedvalidator = 0.0.0DEV-r1049
199 fixtures = 0.3.9
200 funkload = 1.16.1
201+# Required by six
202+futures = 2.2.0
203 grokcore.component = 1.6
204 html5browser = 0.0.9
205 httplib2 = 0.8
206@@ -102,15 +104,15 @@
207 # reapplied a patch from wgrant to get codehosting going again.
208 python-openid = 2.2.5-fix1034376
209 python-subunit = 0.0.8beta
210-python-swiftclient = 1.5.0
211+python-swiftclient = 2.3.1
212 rabbitfixture = 0.3.5
213-requests = 1.2.3
214+requests = 2.5.1
215 s4 = 0.1.2
216 setproctitle = 1.1.7
217 setuptools-git = 1.0
218 simplejson = 3.1.3
219 SimpleTAL = 4.3
220-six = 1.3.0
221+six = 1.9.0
222 soupmatchers = 0.2
223 sourcecodegen = 0.6.14
224 # lp:~launchpad-committers/storm/with-without-datetime