Bazaar

Merge lp:~jameinel/bzr/test-traceback-767177 into lp:bzr

  1. test-traceback-767177
  2. Merge into bzr.dev
Proposed by John A Meinel
Status: Merged
Merge reported by: Martin Packman
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/test-traceback-767177
Merge into: lp:bzr
Diff against target: 227 lines (+71/-44)
7 files modified
bzr (+2/-2)
bzrlib/tests/per_transport.py (+13/-4)
bzrlib/transport/__init__.py (+34/-31)
bzrlib/transport/sftp.py (+2/-5)
doc/en/release-notes/bzr-2.4.txt (+9/-0)
doc/en/whats-new/whats-new-in-2.4.txt (+9/-0)
setup.py (+2/-2)
To merge this branch: bzr merge lp:~jameinel/bzr/test-traceback-767177
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Martin Packman (community) Abstain
Jelmer Vernooij (community) Approve
Review via email: mp+58505@code.launchpad.net

Commit message

Be more agressive about closing file handles. (bug #767177). This patch breaks python-2.4 compatibility.

Description of the change

With this patch, I no longer get ugly Thread tracebacks running:
  bzr selftest -s bt.per_transport --parallel=fork

Basically it does 3 things:

1) Break compatibility with python2.4 by using a try/finally in a generator
2) Be more aggressive about calling fp.close() in readv methods.
3) Handle the cases where we were calling get_multi() but not doing anything with the returned File objects, leaving them with both a pending _prefetch() call and a pending _close() call.

I started a mailing list discussion about the python-2.4 breakage, and I expect to block until that thread seems to have petered out. But I think all of this is worthwhile, and makes our test suite nicer.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

+1 on the code and breaking 2.4 compatibility. Perhaps this should stall for a bit to give people time to object on the mailing list.

review: Approve
Revision history for this message
Martin Packman (gz) wrote :

I don't really like the semantics here, but it's mostly inherited I guess. Making the close call more deterministic is good, but there's too much indentation already.

As _seek_and_read is very nearly a static method, and doubles as a cache, wouldn't pulling it out into a separate iterator class make it easier to follow?

review: Abstain
Revision history for this message
Martin Pool (mbp) wrote :

I'm fine with dropping 2.4 (as I said on the list) but to do it tidily you should:
 * mention it in whatsnew
 * update the check in the bzr script
 * I think update the declaration in setup.py

review: Needs Fixing
lp:~jameinel/bzr/test-traceback-767177 updated
5809. By John A Meinel

Martin rightly pointed out that we needed more work done to drop 2.4 compatibility.

5810. By John A Meinel

Bug #767177 is a dupe of bug #656170, so update the numbers accordingly.

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/21/2011 02:45 AM, Martin Pool wrote:
> Review: Needs Fixing
> I'm fine with dropping 2.4 (as I said on the list) but to do it tidily you should:
> * mention it in whatsnew
> * update the check in the bzr script
> * I think update the declaration in setup.py

Done, though I missed setup.py, I'll try to hit that tomorrow.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2wSJMACgkQJdeBCYSNAAP7YACdGuFiEq+2iP8FwpLgw0CbHDWH
oTAAniFqi4Y1Mown26retIdx9OMvHQvB
=vAFA
-----END PGP SIGNATURE-----

lp:~jameinel/bzr/test-traceback-767177 updated
5811. By John A Meinel

Update setup.py and release-notes to say python 2.6

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/21/2011 02:20 AM, Martin [gz] wrote:
> Review: Abstain
> I don't really like the semantics here, but it's mostly inherited I guess. Making the close call more deterministic is good, but there's too much indentation already.
>
> As _seek_and_read is very nearly a static method, and doubles as a cache, wouldn't pulling it out into a separate iterator class make it easier to follow?

So yes and no. We could certainly rewrite the whole method into an Iterator.

It is also true that we don't really need it to act like a cache
anymore. The use cases we used to have for temporary caching to iterate
in a specific order no longer apply. BTreeGraphIndex et al are perfectly
happy reading in sorted/arbitrary order.

I think the biggest thing for me, is what is worth it for *developer*
time. I think the method is a bit ugly as it stands. But I can write
try/yield/finally in 10 minutes, or I can take 3-4 hours to rewrite it
into a separate tested class, etc.

And that, I think, is why I like going away from python 2.4. Not having
to *think* about how to write it when there is cleaner syntax, and there
aren't a lot of people who need 2.4 anymore.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2xWe4ACgkQJdeBCYSNAAPL5QCdG4gn4ewVabTPAYEUOgmC6Sbo
9rIAoNXrS0tLOyxQ740SQNOk82oQBXsQ
=8vVJ
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

There was a big thread "Breaking python2.4 compatibility for bzr-2.4".

I think the actual patch here will be ok with python2.5 so I think the shortest path is to change it to require only py2.5 or later, and then land it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzr'
2--- bzr 2011-03-11 10:58:18 +0000
3+++ bzr 2011-04-22 10:32:35 +0000
4@@ -31,8 +31,8 @@
5 version_info = 1, 5 # 1.5 or older
6
7 REINVOKE = "__BZR_REINVOKE"
8-NEED_VERS = (2, 4)
9-KNOWN_PYTHONS = ('python2.4', 'python2.5', 'python2.6')
10+NEED_VERS = (2, 6)
11+KNOWN_PYTHONS = ('python2.6', 'python2.7')
12
13 if version_info < NEED_VERS:
14 if not os.environ.has_key(REINVOKE):
15
16=== modified file 'bzrlib/tests/per_transport.py'
17--- bzrlib/tests/per_transport.py 2011-04-14 07:53:38 +0000
18+++ bzrlib/tests/per_transport.py 2011-04-22 10:32:35 +0000
19@@ -207,8 +207,17 @@
20 ]
21 self.build_tree(files, transport=t, line_endings='binary')
22 self.assertRaises(NoSuchFile, t.get, 'c')
23- self.assertListRaises(NoSuchFile, t.get_multi, ['a', 'b', 'c'])
24- self.assertListRaises(NoSuchFile, t.get_multi, iter(['a', 'b', 'c']))
25+ def iterate_and_close(func, *args):
26+ for f in func(*args):
27+ # We call f.read() here because things like paramiko actually
28+ # spawn a thread to prefetch the content, which we want to
29+ # consume before we close the handle.
30+ content = f.read()
31+ f.close()
32+ self.assertRaises(NoSuchFile, iterate_and_close,
33+ t.get_multi, ['a', 'b', 'c'])
34+ self.assertRaises(NoSuchFile, iterate_and_close,
35+ t.get_multi, iter(['a', 'b', 'c']))
36
37 def test_get_directory_read_gives_ReadError(self):
38 """consistent errors for read() on a file returned by get()."""
39@@ -1079,8 +1088,8 @@
40 self.assertListRaises(NoSuchFile, t.stat_multi, iter(['a', 'c', 'd']))
41 self.build_tree(['subdir/', 'subdir/file'], transport=t)
42 subdir = t.clone('subdir')
43- subdir.stat('./file')
44- subdir.stat('.')
45+ st = subdir.stat('./file')
46+ st = subdir.stat('.')
47
48 def test_hardlink(self):
49 from stat import ST_NLINK
50
51=== modified file 'bzrlib/transport/__init__.py'
52--- bzrlib/transport/__init__.py 2011-04-07 10:36:24 +0000
53+++ bzrlib/transport/__init__.py 2011-04-22 10:32:35 +0000
54@@ -287,6 +287,11 @@
55 # where the biggest benefit between combining reads and
56 # and seeking is. Consider a runtime auto-tune.
57 _bytes_to_read_before_seek = 0
58+ # By default, we won't force .close() to be synchronous for remote readv.
59+ # (SFTP in particular has an async .close() call.) However, when running
60+ # the test suite, it would be really nice to have them be synchronous, to
61+ # keep things clean.
62+ _synchronous_close = False
63
64 def __init__(self, base):
65 super(Transport, self).__init__()
66@@ -672,7 +677,9 @@
67
68 This uses _coalesce_offsets to issue larger reads and fewer seeks.
69
70- :param fp: A file-like object that supports seek() and read(size)
71+ :param fp: A file-like object that supports seek() and read(size).
72+ Note that implementations are allowed to call .close() on this file
73+ handle, so don't trust that you can use it for other work.
74 :param offsets: A list of offsets to be read from the given file.
75 :return: yield (pos, data) tuples for each request
76 """
77@@ -689,37 +696,33 @@
78
79 # Cache the results, but only until they have been fulfilled
80 data_map = {}
81- for c_offset in coalesced:
82- # TODO: jam 20060724 it might be faster to not issue seek if
83- # we are already at the right location. This should be
84- # benchmarked.
85- fp.seek(c_offset.start)
86- data = fp.read(c_offset.length)
87- if len(data) < c_offset.length:
88- raise errors.ShortReadvError(relpath, c_offset.start,
89- c_offset.length, actual=len(data))
90- for suboffset, subsize in c_offset.ranges:
91- key = (c_offset.start+suboffset, subsize)
92- data_map[key] = data[suboffset:suboffset+subsize]
93+ try:
94+ for c_offset in coalesced:
95+ # TODO: jam 20060724 it might be faster to not issue seek if
96+ # we are already at the right location. This should be
97+ # benchmarked.
98+ fp.seek(c_offset.start)
99+ data = fp.read(c_offset.length)
100+ if len(data) < c_offset.length:
101+ raise errors.ShortReadvError(relpath, c_offset.start,
102+ c_offset.length, actual=len(data))
103+ for suboffset, subsize in c_offset.ranges:
104+ key = (c_offset.start+suboffset, subsize)
105+ data_map[key] = data[suboffset:suboffset+subsize]
106
107- # Now that we've read some data, see if we can yield anything back
108- while cur_offset_and_size in data_map:
109- this_data = data_map.pop(cur_offset_and_size)
110- this_offset = cur_offset_and_size[0]
111- try:
112- cur_offset_and_size = offset_stack.next()
113- except StopIteration:
114- # Close the file handle as there will be no more data
115- # The handle would normally be cleaned up as this code goes
116- # out of scope, but as we are a generator, not all code
117- # will re-enter once we have consumed all the expected
118- # data. For example:
119- # zip(range(len(requests)), readv(foo, requests))
120- # Will stop because the range is done, and not run the
121- # cleanup code for the readv().
122- fp.close()
123- cur_offset_and_size = None
124- yield this_offset, this_data
125+ # Now that we've read some data, see if we can yield anything back
126+ while cur_offset_and_size in data_map:
127+ this_data = data_map.pop(cur_offset_and_size)
128+ this_offset = cur_offset_and_size[0]
129+ try:
130+ cur_offset_and_size = offset_stack.next()
131+ except StopIteration:
132+ cur_offset_and_size = None
133+ yield this_offset, this_data
134+ finally:
135+ # Note that this is not python2.4 compatible as try/finally in
136+ # generators was added in python 2.5
137+ fp.close()
138
139 def _sort_expand_and_combine(self, offsets, upper_limit):
140 """Helper for readv.
141
142=== modified file 'bzrlib/transport/sftp.py'
143--- bzrlib/transport/sftp.py 2010-08-24 13:03:18 +0000
144+++ bzrlib/transport/sftp.py 2011-04-22 10:32:35 +0000
145@@ -281,6 +281,8 @@
146 buffered = buffered[buffered_offset:]
147 buffered_data = [buffered]
148 buffered_len = len(buffered)
149+ # now that the data stream is done, close the handle
150+ fp.close()
151 if buffered_len:
152 buffered = ''.join(buffered_data)
153 del buffered_data[:]
154@@ -421,11 +423,6 @@
155 :param relpath: The relative path to the file
156 """
157 try:
158- # FIXME: by returning the file directly, we don't pass this
159- # through to report_activity. We could try wrapping the object
160- # before it's returned. For readv and get_bytes it's handled in
161- # the higher-level function.
162- # -- mbp 20090126
163 path = self._remote_path(relpath)
164 f = self._get_sftp().file(path, mode='rb')
165 if self._do_prefetch and (getattr(f, 'prefetch', None) is not None):
166
167=== modified file 'doc/en/release-notes/bzr-2.4.txt'
168--- doc/en/release-notes/bzr-2.4.txt 2011-04-21 13:22:08 +0000
169+++ doc/en/release-notes/bzr-2.4.txt 2011-04-22 10:32:35 +0000
170@@ -15,6 +15,11 @@
171
172 .. These may require users to change the way they use Bazaar.
173
174+* We have finally broken compatibility with python 2.4. From bzr-2.4
175+ onward we only maintain python-2.6+ compatibility. Note that stable
176+ update releases will continue to be made on older versions.
177+ (John Arbash Meinel)
178+
179 * Two command synonyms for ``bzr branch`` have been deprecated, to avoid
180 confusion and to allow the names to later be reused. The removed names
181 are: ``get`` and ``clone``. (Martin Pool, #506265)
182@@ -83,6 +88,10 @@
183 know about so far have been fixed, but there may be fallout for edge
184 cases that we are missing. (John Arbash Meinel, #759091)
185
186+* ``SFTPTransport`` is more pro-active about closing file-handles. This
187+ reduces the chance of having threads fail from async requests while
188+ running the test suite. (John Arbash Meinel, #656170)
189+
190 * Standalone bzr.exe installation on Windows: user can put additional python
191 libraries into ``site-packages`` subdirectory of the installation directory,
192 this might be required for "installing" extra dependencies for some plugins.
193
194=== modified file 'doc/en/whats-new/whats-new-in-2.4.txt'
195--- doc/en/whats-new/whats-new-in-2.4.txt 2011-03-27 11:04:46 +0000
196+++ doc/en/whats-new/whats-new-in-2.4.txt 2011-04-22 10:32:35 +0000
197@@ -16,6 +16,15 @@
198 2.1, 2.2 and 2.3, and can read and write repositories generated by all
199 previous versions.
200
201+Python 2.6 and 2.7 compatibility only
202+*************************************
203+
204+Starting with Bazaar 2.4, we will no longer be supporting python 2.4 or
205+2.5. Python 2.4 is no longer supported by upstream python development, and
206+all major platforms have already switched to python 2.6. This allows
207+Bazaar to have cleaner syntax internally, by using constructs that are
208+only available with newer versions of python.
209+
210 External merge tools
211 ********************
212
213
214=== modified file 'setup.py'
215--- setup.py 2010-12-26 13:19:11 +0000
216+++ setup.py 2011-04-22 10:32:35 +0000
217@@ -11,8 +11,8 @@
218 import sys
219 import copy
220
221-if sys.version_info < (2, 4):
222- sys.stderr.write("[ERROR] Not a supported Python version. Need 2.4+\n")
223+if sys.version_info < (2, 6):
224+ sys.stderr.write("[ERROR] Not a supported Python version. Need 2.6+\n")
225 sys.exit(1)
226
227 # NOTE: The directory containing setup.py, whether run by 'python setup.py' or