Merge lp:~mvo/python-apt/mvo into lp:~ubuntu-core-dev/python-apt/ubuntu

Proposed by Michael Vogt on 2012-10-15
Status: Merged
Merged at revision: 493
Proposed branch: lp:~mvo/python-apt/mvo
Merge into: lp:~ubuntu-core-dev/python-apt/ubuntu
Diff against target: 322 lines (+166/-9) (has conflicts)
7 files modified
apt/auth.py (+2/-1)
apt/cache.py (+23/-0)
debian/changelog (+76/-0)
debian/control (+8/-0)
python/generic.h (+4/-0)
python/tag.cc (+2/-2)
tests/test_apt_cache.py (+51/-6)
Text conflict in debian/changelog
Text conflict in debian/control
To merge this branch: bzr merge lp:~mvo/python-apt/mvo
Reviewer Review Type Date Requested Status
Jason Conti (community) Approve on 2012-10-15
Ubuntu Core Development Team 2012-10-15 Pending
Review via email: mp+129630@code.launchpad.net

Description of the change

The work from Jason Conti to fix leaking FDs

To post a comment you must log in.
lp:~mvo/python-apt/mvo updated on 2012-10-15
619. By Michael Vogt on 2012-10-15

apt/cache.py: add comment

Jason Conti (jconti) wrote :

Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.

I like the other additional tests, and actually had a very similar test to test_cache_close_download_fails() but it didn't occur to me to include it for some reason. It also appears I completely missed the case about leaking fds on cache reopen, thanks for fixing it.

review: Approve
Michael Vogt (mvo) wrote :

On Mon, Oct 15, 2012 at 05:08:22PM -0000, Jason Conti wrote:
> Review: Approve
>
> Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.

Thanks a lot! Oh, interessting. I had hoped that the gc.collect()
would ensure that its deleted and thereforce closed. Does the test
fails reliable for you? If so, any hints what I can do trying to
reproduce the issue?

> I like the other additional tests, and actually had a very similar test to test_cache_close_download_fails() but it didn't occur to me to include it for some reason. It also appears I completely missed the case about leaking fds on cache reopen, thanks for fixing it.

Great, thanks for the review!

Cheers,
 Michael

lp:~mvo/python-apt/mvo updated on 2012-10-17
620. By Michael Vogt on 2012-10-17

tests/test_apt_cache.py: delete test_cache_delete_leasks_fds() as its not reliable

Michael Vogt (mvo) wrote :

On Mon, Oct 15, 2012 at 05:08:22PM -0000, Jason Conti wrote:
> Review: Approve
>
> Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.
[..]

Nevermind, its also failing for me now, not sure why I did not see
that before. I deleted the test again.

Cheers,
 Michael

lp:~mvo/python-apt/mvo updated on 2012-10-23
621. By Michael Vogt on 2012-10-23

build fixes for python3.3

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'apt/auth.py'
2--- apt/auth.py 2012-10-12 09:06:34 +0000
3+++ apt/auth.py 2012-10-23 10:39:23 +0000
4@@ -78,7 +78,8 @@
5 stderr=subprocess.PIPE)
6
7 content = kwargs.get("stdin", None)
8- if isinstance(content, unicode):
9+ # py2 needs this encoded, py3.3 will crash if it is
10+ if isinstance(content, unicode) and sys.version_info[:2] < (3, 3):
11 content = content.encode("utf-8")
12
13 output, stderr = proc.communicate(content)
14
15=== modified file 'apt/cache.py'
16--- apt/cache.py 2012-04-17 19:17:10 +0000
17+++ apt/cache.py 2012-10-23 10:39:23 +0000
18@@ -42,6 +42,9 @@
19 class LockFailedException(IOError):
20 """Exception that is thrown when locking fails."""
21
22+class CacheClosedException(Exception):
23+ """Exception that is thrown when the cache is used after close()."""
24+
25
26 class Cache(object):
27 """Dictionary-like package cache.
28@@ -139,6 +142,8 @@
29 """
30 if progress is None:
31 progress = apt.progress.base.OpProgress()
32+ # close old cache on (re)open
33+ self.close()
34 self.op_progress = progress
35 self._run_callbacks("cache_pre_open")
36
37@@ -172,6 +177,20 @@
38 progress.done()
39 self._run_callbacks("cache_post_open")
40
41+ def close(self):
42+ """ Close the package cache """
43+ # explicitely free the FDs that _records has open
44+ del self._records
45+ self._records = None
46+
47+ def __enter__(self):
48+ """ Enter the with statement """
49+ return self
50+
51+ def __exit__(self, exc_type, exc_value, traceback):
52+ """ Exit the with statement """
53+ self.close()
54+
55 def __getitem__(self, key):
56 """ look like a dictionary (get key) """
57 try:
58@@ -238,6 +257,8 @@
59 @property
60 def required_download(self):
61 """Get the size of the packages that are required to download."""
62+ if self._records is None:
63+ raise CacheClosedException("Cache object used after close() called")
64 pm = apt_pkg.PackageManager(self._depcache)
65 fetcher = apt_pkg.Acquire()
66 pm.get_archives(fetcher, self._list, self._records)
67@@ -288,6 +309,8 @@
68
69 def _fetch_archives(self, fetcher, pm):
70 """ fetch the needed archives """
71+ if self._records is None:
72+ raise CacheClosedException("Cache object used after close() called")
73
74 # get lock
75 lockfile = apt_pkg.config.find_dir("Dir::Cache::Archives") + "lock"
76
77=== modified file 'debian/changelog'
78--- debian/changelog 2012-10-12 09:11:14 +0000
79+++ debian/changelog 2012-10-23 10:39:23 +0000
80@@ -1,3 +1,4 @@
81+<<<<<<< TREE
82 python-apt (0.8.8ubuntu1) UNRELEASED; urgency=low
83
84 NOTE: THIS is intended for R, use lp:~ubuntu-core-dev/python-apt/quantal
85@@ -107,6 +108,81 @@
86
87 -- Michael Vogt <michael.vogt@ubuntu.com> Fri, 29 Jun 2012 16:05:52 +0200
88
89+=======
90+python-apt (0.8.8.1) UNRELEASED; urgency=low
91+
92+ [ Michael Vogt ]
93+ * python/tag.cc:
94+ - make TagSecString_FromStringAndSize, TagSecString_FromString
95+ static, thanks to jcristau
96+ * python/cache.cc:
97+ - add "Codename" to PackageFile object
98+ * add dep8 style autopkgtest support
99+ * build fixes for python3.3
100+
101+ [ Jason Conti ]
102+ * lp:~jconti/python-apt/closeable-cache:
103+ - add apt.Cache.close() method
104+
105+ -- Michael Vogt <michael.vogt@ubuntu.com> Mon, 15 Oct 2012 10:03:21 +0200
106+
107+python-apt (0.8.8) unstable; urgency=low
108+
109+ [ Program translation updates ]
110+ * po/pl.po: Polish (Michał Kułach) (closes: #684308)
111+ * po/da.po: Danish (Joe Hansen) (closes: #689827)
112+
113+ [ Michael Vogt ]
114+ * merged lp:~sampo555/python-apt/fix_1042916 reuse existing but
115+ disabled sources.list entries instead of duplicating them.
116+ Thanks to "sampo555", LP: #1042916
117+ * lp:~mvo/python-apt/fix-debfile-crash:
118+ - fix crash on missing candidates in the multiarch check
119+ * lp:~mvo/python-apt/recv-key-lp1016643:
120+ - Only support long (v4) keyids when downloading keys and
121+ check the keys fingerprint before importing. This avoids
122+ man-in-the-middle attacks (LP: #1016643)
123+ * consolidate tests/test_lp1030278.py into the new
124+ tests/test_size_to_str.py
125+ * apt/auth.py:
126+ - support importing long keyids with leading 0x and mixed case
127+ * debian/control:
128+ - build-depend on python-unittest2 to get "with TestCase.assertRaises"
129+ support in python2.6
130+
131+ [ Barry Warsaw ]
132+ * python/string.cc, tests/test_lp1030278.py: Fix StrSizeToStr() so that
133+ 1) it first checks for PyLong-ness so that in Python 3 on i386, it
134+ will be able to convert larger numbers (via doubles rather than ints);
135+ 2) before doing the conversions through the apt API, check to see if a
136+ Python exception occurred, e.g. OverflowError, and return an error
137+ condition in that case instead of masking it. (LP: #1030278)
138+
139+ [ James Hunt ]
140+ * python/cache.cc: PkgCacheGetIsMultiArch(): Return calculated
141+ value rather than a random one.
142+ * lp:~jamesodhunt/python-apt/test-for-size_to_str:
143+ - add test for size_to_str() to help with finding LP: #1030278
144+
145+ -- Michael Vogt <mvo@debian.org> Fri, 12 Oct 2012 10:47:11 +0200
146+
147+python-apt (0.8.7) unstable; urgency=low
148+
149+ [ Translation updates ]
150+ * po/es.po: Spanish translation updated by Omar Campagne (closes: #679285)
151+ * po/ja.po: Japanese translation updated by Kenshi Muto (closes: #679652)
152+
153+ [ Jakub Wilk ]
154+ * Fix typos: the the -> the (closes: #679432)
155+
156+ [ Julian Andres Klode ]
157+ * apt/auth.py:
158+ - Do not merge stdout and stderr (Closes: #678706)
159+ - Forward stderr from apt-key to our stderr if non-empty
160+
161+ -- Julian Andres Klode <jak@debian.org> Mon, 30 Jul 2012 13:29:17 +0200
162+
163+>>>>>>> MERGE-SOURCE
164 python-apt (0.8.6) unstable; urgency=low
165
166 [ Michael Vogt ]
167
168=== modified file 'debian/control'
169--- debian/control 2012-10-12 09:06:34 +0000
170+++ debian/control 2012-10-23 10:39:23 +0000
171@@ -18,11 +18,19 @@
172 python3-all-dbg (>= 3.1.2-6~),
173 python-distutils-extra (>= 2.0),
174 python-sphinx (>= 0.5),
175+<<<<<<< TREE
176 python-debian
177 Vcs-Bzr: http://code.launchpad.net/~ubuntu-core-dev/python-apt/ubuntu
178 XS-Debian-Vcs-Bzr: http://bzr.debian.org/apt/python-apt/debian-sid
179 XS-Debian-Vcs-Browser: http://bzr.debian.org/loggerhead/apt/python-apt/debian-sid/changes
180 XS-Testsuite: autopkgtest
181+=======
182+ python-debian,
183+ python-unittest2
184+Vcs-Bzr: http://bzr.debian.org/apt/python-apt/debian-sid
185+Vcs-Browser: http://bzr.debian.org/loggerhead/apt/python-apt/debian-sid/changes
186+XS-Testsuite: autopkgtest
187+>>>>>>> MERGE-SOURCE
188
189 Package: python-apt
190 Architecture: any
191
192=== modified file 'python/generic.h'
193--- python/generic.h 2011-08-01 07:29:25 +0000
194+++ python/generic.h 2012-10-23 10:39:23 +0000
195@@ -80,10 +80,14 @@
196
197 static inline const char *PyUnicode_AsString(PyObject *op) {
198 // Convert to bytes object, using the default encoding.
199+#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
200+ return PyUnicode_AsUTF8(op);
201+#else
202 // Use Python-internal API, there is no other way to do this
203 // without a memory leak.
204 PyObject *bytes = _PyUnicode_AsDefaultEncodedString(op, 0);
205 return bytes ? PyBytes_AS_STRING(bytes) : 0;
206+#endif
207 }
208
209 // Convert any type of string based object to a const char.
210
211=== modified file 'python/tag.cc'
212--- python/tag.cc 2012-06-17 21:11:16 +0000
213+++ python/tag.cc 2012-10-23 10:39:23 +0000
214@@ -74,7 +74,7 @@
215 PyString_FromStringAndSize((v), (len))
216 #define TagSecString_FromString(self, v) PyString_FromString(v)
217 #else
218-PyObject *TagSecString_FromStringAndSize(PyObject *self, const char *v,
219+static PyObject *TagSecString_FromStringAndSize(PyObject *self, const char *v,
220 Py_ssize_t len) {
221 TagSecData *Self = (TagSecData *)self;
222 if (Self->Bytes)
223@@ -85,7 +85,7 @@
224 return PyUnicode_FromStringAndSize(v, len);
225 }
226
227-PyObject *TagSecString_FromString(PyObject *self, const char *v) {
228+static PyObject *TagSecString_FromString(PyObject *self, const char *v) {
229 TagSecData *Self = (TagSecData *)self;
230 if (Self->Bytes)
231 return PyBytes_FromString(v);
232
233=== modified file 'tests/test_apt_cache.py'
234--- tests/test_apt_cache.py 2011-12-09 08:16:08 +0000
235+++ tests/test_apt_cache.py 2012-10-23 10:39:23 +0000
236@@ -7,19 +7,27 @@
237 # are permitted in any medium without royalty provided the copyright
238 # notice and this notice are preserved.
239 """Unit tests for verifying the correctness of check_dep, etc in apt_pkg."""
240+
241+import glob
242+import logging
243 import os
244+import shutil
245+import sys
246 import tempfile
247 import unittest
248
249+if sys.version_info[0] == 2 and sys.version_info[1] == 6:
250+ from unittest2 import TestCase
251+else:
252+ from unittest import TestCase
253+
254+
255 from test_all import get_library_dir
256-import sys
257 sys.path.insert(0, get_library_dir())
258
259 import apt
260 import apt_pkg
261-import shutil
262-import glob
263-import logging
264+
265
266 def if_sources_list_is_readable(f):
267 def wrapper(*args, **kwargs):
268@@ -29,7 +37,17 @@
269 logging.warn("skipping '%s' because sources.list is not readable" % f)
270 return wrapper
271
272-class TestAptCache(unittest.TestCase):
273+
274+def get_open_file_descriptors():
275+ try:
276+ fds = os.listdir("/proc/self/fd")
277+ except OSError:
278+ logging.warn("failed to list /proc/self/fd")
279+ return set([])
280+ return set(map(int, fds))
281+
282+
283+class TestAptCache(TestCase):
284 """ test the apt cache """
285
286 def setUp(self):
287@@ -68,7 +86,34 @@
288 self.assertEqual(r['Package'], pkg.shortname)
289 self.assertTrue('Version' in r)
290 self.assertTrue(len(r['Description']) > 0)
291- self.assertTrue(str(r).startswith('Package: %s\n' % pkg.shortname))
292+ self.assertTrue(
293+ str(r).startswith('Package: %s\n' % pkg.shortname))
294+
295+ @if_sources_list_is_readable
296+ def test_cache_close_leak_fd(self):
297+ fds_before_open = get_open_file_descriptors()
298+ cache = apt.Cache()
299+ opened_fd = get_open_file_descriptors().difference(fds_before_open)
300+ cache.close()
301+ fds_after_close = get_open_file_descriptors()
302+ unclosed_fd = opened_fd.intersection(fds_after_close)
303+ self.assertEqual(fds_before_open, fds_after_close)
304+ self.assertEqual(unclosed_fd, set())
305+
306+ def test_cache_open_twice_leaks_fds(self):
307+ cache = apt.Cache()
308+ fds_before_open = get_open_file_descriptors()
309+ cache.open()
310+ fds_after_open_twice = get_open_file_descriptors()
311+ self.assertEqual(fds_before_open, fds_after_open_twice)
312+
313+ @if_sources_list_is_readable
314+ def test_cache_close_download_fails(self):
315+ cache = apt.Cache()
316+ self.assertEqual(cache.required_download, 0)
317+ cache.close()
318+ with self.assertRaises(apt.cache.CacheClosedException):
319+ cache.required_download
320
321 def test_get_provided_packages(self):
322 apt.apt_pkg.config.set("Apt::architecture", "i386")

Subscribers

People subscribed via source and target branches

to all changes: