Patchwork [2,of,7,iterbatch] wireproto: document quirk of _callstream between http and ssh

login
register
mail settings
Submitter Augie Fackler
Date March 8, 2016, 4:25 a.m.
Message ID <071d05cce9156434dee4.1457411138@147.17.16.172.in-addr.arpa>
Download mbox | patch
Permalink /patch/13658/
State Accepted
Delegated to: Pierre-Yves David
Headers show

Comments

Augie Fackler - March 8, 2016, 4:25 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1456946323 18000
#      Wed Mar 02 14:18:43 2016 -0500
# Node ID 071d05cce9156434dee4696287119f284631701b
# Parent  5fd58c8c55d052c671a4725d01f27fb1c14353a9
# EXP-Topic batch
wireproto: document quirk of _callstream between http and ssh

This tripped me up when trying to use it, so it feels like we should
document this to avoid future pain.
Martin von Zweigbergk - March 8, 2016, 11:54 p.m.
On Mon, Mar 7, 2016 at 8:25 PM, Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1456946323 18000
> #      Wed Mar 02 14:18:43 2016 -0500
> # Node ID 071d05cce9156434dee4696287119f284631701b
> # Parent  5fd58c8c55d052c671a4725d01f27fb1c14353a9
> # EXP-Topic batch
> wireproto: document quirk of _callstream between http and ssh
>
> This tripped me up when trying to use it, so it feels like we should
> document this to avoid future pain.
>
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -396,9 +396,12 @@ class wirepeer(peer.peerrepository):
>      def _callstream(self, cmd, **args):
>          """execute <cmd> on the server
>
> -        The command is expected to return a stream.
> +        The command is expected to return a stream. Note that if the
> +        command doesn't return a stream, _callstream behaves
> +        differently for ssh and http peers.
>
> -        returns the server reply as a file like object."""
> +        returns the server reply as a file like object.
> +        """

I find the documentation very confusing, but that's mostly unrelated
to your patch. So the method is expected to return a stream, but it
doesn't have to? What does that mean? And is a "file like object" a
stream?

>          raise NotImplementedError()
>
>      def _callcompressable(self, cmd, **args):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - March 9, 2016, 1:02 a.m.
> On Mar 8, 2016, at 6:54 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> On Mon, Mar 7, 2016 at 8:25 PM, Augie Fackler <raf@durin42.com> wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1456946323 18000
>> #      Wed Mar 02 14:18:43 2016 -0500
>> # Node ID 071d05cce9156434dee4696287119f284631701b
>> # Parent  5fd58c8c55d052c671a4725d01f27fb1c14353a9
>> # EXP-Topic batch
>> wireproto: document quirk of _callstream between http and ssh
>> 
>> This tripped me up when trying to use it, so it feels like we should
>> document this to avoid future pain.
>> 
>> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
>> --- a/mercurial/wireproto.py
>> +++ b/mercurial/wireproto.py
>> @@ -396,9 +396,12 @@ class wirepeer(peer.peerrepository):
>>     def _callstream(self, cmd, **args):
>>         """execute <cmd> on the server
>> 
>> -        The command is expected to return a stream.
>> +        The command is expected to return a stream. Note that if the
>> +        command doesn't return a stream, _callstream behaves
>> +        differently for ssh and http peers.
>> 
>> -        returns the server reply as a file like object."""
>> +        returns the server reply as a file like object.
>> +        """
> 
> I find the documentation very confusing, but that's mostly unrelated
> to your patch. So the method is expected to return a stream, but it
> doesn't have to? What does that mean? And is a "file like object" a
> stream?

httppeer returns a file-like object which hits EOF when the specific response ends. This is the same wether or not the called method is defined to be a streaming method.

sshpeer returns a file-like object, which is the stdout of the ssh subprocess. As a result, it *does not* hit EOF at the end of a given response. If the method is defined to be a streaming method, the response body must be self-describing at least as far as content length. If the method is *not* a streaming method (as is the case for the batch() method today), then the sshpeer first emits ā€œ%d\nā€ % len(response), and then the client knows to read that many bytes after the newline.

A good future cleanup would probably be to introduce a streambatch() wireproto method and then deprecate batch() so that the batching behavior can be the same on both ssh and http.

> 
>>         raise NotImplementedError()
>> 
>>     def _callcompressable(self, cmd, **args):
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - March 10, 2016, 2:30 p.m.
On 03/09/2016 01:02 AM, Augie Fackler wrote:
>
>> On Mar 8, 2016, at 6:54 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
>>
>> On Mon, Mar 7, 2016 at 8:25 PM, Augie Fackler <raf@durin42.com> wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie@google.com>
>>> # Date 1456946323 18000
>>> #      Wed Mar 02 14:18:43 2016 -0500
>>> # Node ID 071d05cce9156434dee4696287119f284631701b
>>> # Parent  5fd58c8c55d052c671a4725d01f27fb1c14353a9
>>> # EXP-Topic batch
>>> wireproto: document quirk of _callstream between http and ssh
>>>
>>> This tripped me up when trying to use it, so it feels like we should
>>> document this to avoid future pain.
>>>
>>> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
>>> --- a/mercurial/wireproto.py
>>> +++ b/mercurial/wireproto.py
>>> @@ -396,9 +396,12 @@ class wirepeer(peer.peerrepository):
>>>      def _callstream(self, cmd, **args):
>>>          """execute <cmd> on the server
>>>
>>> -        The command is expected to return a stream.
>>> +        The command is expected to return a stream. Note that if the
>>> +        command doesn't return a stream, _callstream behaves
>>> +        differently for ssh and http peers.
>>>
>>> -        returns the server reply as a file like object."""
>>> +        returns the server reply as a file like object.
>>> +        """
>>
>> I find the documentation very confusing, but that's mostly unrelated
>> to your patch. So the method is expected to return a stream, but it
>> doesn't have to? What does that mean? And is a "file like object" a
>> stream?
>
> httppeer returns a file-like object which hits EOF when the specific response ends. This is the same wether or not the called method is defined to be a streaming method.
>
> sshpeer returns a file-like object, which is the stdout of the ssh subprocess. As a result, it *does not* hit EOF at the end of a given response. If the method is defined to be a streaming method, the response body must be self-describing at least as far as content length. If the method is *not* a streaming method (as is the case for the batch() method today), then the sshpeer first emits ā€œ%d\nā€ % len(response), and then the client knows to read that many bytes after the newline.
>
> A good future cleanup would probably be to introduce a streambatch() wireproto method and then deprecate batch() so that the batching behavior can be the same on both ssh and http.

Patches 1 and 2 are on the clowncopter.

It would probably be useful to follow up with a documentation update 
mentionning this ssh/http business (small layer violation, but I think 
it is useful)

Cheers.

Patch

diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -396,9 +396,12 @@  class wirepeer(peer.peerrepository):
     def _callstream(self, cmd, **args):
         """execute <cmd> on the server
 
-        The command is expected to return a stream.
+        The command is expected to return a stream. Note that if the
+        command doesn't return a stream, _callstream behaves
+        differently for ssh and http peers.
 
-        returns the server reply as a file like object."""
+        returns the server reply as a file like object.
+        """
         raise NotImplementedError()
 
     def _callcompressable(self, cmd, **args):