[MERGE] Add call_with_body_stream to _SmartClient; add support for streamed request bodies to the server.

Andrew Bennetts andrew.bennetts at canonical.com
Tue Jan 27 05:56:00 GMT 2009


John Arbash Meinel wrote:
> John Arbash Meinel has voted tweak.
> Status is now: Conditionally approved
> Comment:
> +
> +    Possible states:
> +     * expecting args
> +     * expecting body (terminated by receiving a post-body status)
> +     * finished
>      """
>
>
> ^- It seems that this is incomplete. I'm not sure what other states are
> available, but at the very least I saw:

Good point.  I've expanded it to read:

    Possible states:
     * args: expecting args
     * body: expecting body (terminated by receiving a post-body status)
     * error: expecting post-body error
     * end: expecting end of message
     * nothing: finished

> I find it a little bit odd that the "accept_body" function is now  
> calling
> "do_chunk" rather than calling "do_body".

This is intentional, although maybe the name of accept_body could be clearer.
accept_body functions in the smart code have always taken incomplete body parts.
Previously this accept_body was just accumulating the body in a string; now it's
up to the SmartServerRequest to decide how to deal with these parts (by default
it accumulates as before).

> v- Having run into this elsewhere:
[...]
> +        self._body_bytes += chunk_bytes
[...]
>
> ^- It would be a lot better if "self._body_bytes" was a list that we  
> appended
> to, and then used ''.join() if we had to. Remember the terrible scaling  
> of
> 'readv()' when it was reading 100MB, 64k chunks at a time.

Ok, done.  The only reason I hadn't already done this was just to try keep this
patch somewhat minimal (this is the current behaviour shifted to a different
class). 

> === modified file 'bzrlib/util/bencode.py'
> --- bzrlib/util/bencode.py      2007-07-13 04:22:17 +0000
> +++ bzrlib/util/bencode.py      2009-01-09 05:52:37 +0000
> @@ -52,7 +52,7 @@
>      while x[f] != 'e':
>          v, f = decode_func[x[f]](x, f)
>          r.append(v)
> -    return (r, f + 1)
> +    return (tuple(r), f + 1)
>
>
> ^- Also, these changes seem specifically incorrect now, as you don't  
> really
> want to universally change the api to return tuples instead of lists. It
> probably just means that this can't land until some form of your other  
> patch
> lands.

Right, I even mentioned it in my preamble to this patch.
<http://bundlebuggy.aaronbentley.com/project/bzr/request/%3C20090107022415.GA2394%40steerpike.home.puzzling.org%3E>
is still waiting for a review :)

Actually, I'll probably just merge the bencode patch in a day or so if it stays
unreviewed; I'm quite sure it's reasonable and safe, and I don't want it to
block this call_with_body_stream patch.

-Andrew.




More information about the bazaar mailing list