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

[Vincent Ladeuil]

>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

    john> ...
    >> 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.
    >> 

Thanks for the detailed explanation.

    >> 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.

Not worth the effort, thanks for tracking down the problematic
cases.

<snip/>

    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.


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

I agree that is the best that can be achieved given the generator
limitations.

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

Perfect for me.

        Vincent