[MERGE] Transport.readv() closes the file handle

[John Arbash Meinel]

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

...

> I *can* say that it is happening, and it is a bit hard to debug or test,
> because it only fails if you are doing something concurrently (which we
> don't often do in bzr) and if you are on a platform that doesn't like
> file handles left open.
> 
> So if you prefer, I can make '.close()' a strictly required attribute,
> but I don't think I can assume all callers run the iterator to
> completion (because of the issue with zip()). I could track down all the
> different places to put readv() first, but rememember that sometimes
> generators are stacked, and that gets tricky to resolve.
> 
> John
> =:->

I ended up going with this code:
> === modified file 'bzrlib/transport/__init__.py'
> --- bzrlib/transport/__init__.py        2008-10-06 06:34:36 +0000
> +++ bzrlib/transport/__init__.py        2008-10-27 15:28:44 +0000
> @@ -688,8 +688,21 @@
>              # Now that we've read some data, see if we can yield anything back
>              while cur_offset_and_size in data_map:
>                  this_data = data_map.pop(cur_offset_and_size)
> -                yield cur_offset_and_size[0], this_data
> -                cur_offset_and_size = offset_stack.next()
> +                this_offset = cur_offset_and_size[0]
> +                try:
> +                    cur_offset_and_size = offset_stack.next()
> +                except StopIteration:
> +                    # Close the file handle as there will be no more data
> +                    # The handle would normally be cleaned up as this code goes
> +                    # out of scope, but as we are a generator, not all code
> +                    # will re-enter once we have consumed all the expected
> +                    # data. For example:
> +                    #   zip(range(len(requests)), readv(foo, requests))
> +                    # Will stop because the range is done, and not run the
> +                    # cleanup code for the readv().
> +                    fp.close()
> +                    cur_offset_and_size = None
> +                yield this_offset, this_data
> 
>      def _sort_expand_and_combine(self, offsets, upper_limit):
>          """Helper for readv.


It means that when the iterator *is* fully consumed, we will reach the
end of the function, rather than exiting from the exception raised by
offset_stack.next()
It also means I don't have to have a counter tracking along with the
iterator. It moves the .next() before the yield, but this is still safe.

I did make fp.close() strictly required, which is a small api change,
but shouldn't actually cause problems.

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

iEYEARECAAYFAkkF4mwACgkQJdeBCYSNAAOp4wCeKivHvydl5yln3AGcRYG5P8Wf
hKwAnjusAM8jirJZNKNJPtZeEsXq/jk6
=kmxo
-----END PGP SIGNATURE-----