Patchwork D2860: wireproto: buffer output frames when in half duplex mode

login
register
mail settings
Submitter phabricator
Date March 14, 2018, 4:43 p.m.
Message ID <differential-rev-PHID-DREV-db64sl3t2kh2f2u4clt2-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29505/
State Superseded
Headers show

Comments

phabricator - March 14, 2018, 4:43 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, when told that a response was ready, the server reactor
  would instruct the caller to send frames immediately. This was OK
  as an initial implementation. But it would not work for half-duplex
  connections where the sender can't receive until all data has been
  transmitted - such as httplib based clients.
  
  In this commit, we teach the reactor that output frames should
  be buffered until end of input is seen. This required a new
  event to inform the reactor of end of input. The result from that
  event will instruct the consumer to send all buffered frames.
  
  The HTTP server is buffered by default.
  
  This change effectively hides the complexity of buffering within
  the reactor so that transports need not be concerned about it.
  This helps keep the transports "dumb" and will make implementing
  multiple requests-responses per atomic exchange (like an HTTP
  request) much simpler.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2860

AFFECTED FILES
  mercurial/wireprotoframing.py
  mercurial/wireprotoserver.py
  tests/test-http-api-httpv2.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 22, 2018, 1:18 a.m.
durin42 added inline comments.

INLINE COMMENTS

> wireprotoserver.py:418-420
> +    # TODO Some HTTP clients are full duplex and can receive data before
> +    # the entire request is transmitted. Figure out a way to indicate support
> +    # for that so we can opt into full duplex mode.

Recent RFCs explicitly disallow early responses with a 2xx series status code. Probably not worth pursuing.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2860

To: indygreg, #hg-reviewers
Cc: durin42, mercurial-devel

Patch

diff --git a/tests/test-http-api-httpv2.t b/tests/test-http-api-httpv2.t
--- a/tests/test-http-api-httpv2.t
+++ b/tests/test-http-api-httpv2.t
@@ -384,11 +384,12 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 117\r\n
+  s>     Content-Length: 136\r\n
   s>     \r\n
   s>     received: 1 1 command1\n
   s>     {"action": "runcommand", "args": {}, "command": "command1", "data": null}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Single argument frame is received
 
@@ -415,13 +416,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 217\r\n
+  s>     Content-Length: 236\r\n
   s>     \r\n
   s>     received: 1 2 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-args"}\n
   s>     received: 2 2 \x03\x00\x05\x00foovalue\n
   s>     {"action": "runcommand", "args": {"foo": "value"}, "command": "command1", "data": null}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Multiple argument frames are received
 
@@ -449,15 +451,16 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 319\r\n
+  s>     Content-Length: 338\r\n
   s>     \r\n
   s>     received: 1 2 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-args"}\n
   s>     received: 2 0 \x03\x00\x05\x00foovalue\n
   s>     {"action": "wantframe", "state": "command-receiving-args"}\n
   s>     received: 2 2 \x04\x00\x04\x00foo1val1\n
   s>     {"action": "runcommand", "args": {"foo": "value", "foo1": "val1"}, "command": "command1", "data": null}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Command with single data frame
 
@@ -484,13 +487,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 199\r\n
+  s>     Content-Length: 218\r\n
   s>     \r\n
   s>     received: 1 4 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-data"}\n
   s>     received: 3 2 data!\n
   s>     {"action": "runcommand", "args": {}, "command": "command1", "data": "data!"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Command with multiple data frames
 
@@ -519,7 +523,7 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 367\r\n
+  s>     Content-Length: 386\r\n
   s>     \r\n
   s>     received: 1 4 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-data"}\n
@@ -529,7 +533,8 @@ 
   s>     {"action": "wantframe", "state": "command-receiving-data"}\n
   s>     received: 3 2 data3\n
   s>     {"action": "runcommand", "args": {}, "command": "command1", "data": "data1data2data3"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Unexpected frame type
 
@@ -556,13 +561,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 181\r\n
+  s>     Content-Length: 200\r\n
   s>     \r\n
   s>     received: 2 0 0\n
   s>     {"action": "error", "message": "expected command frame; got 2"}\n
   s>     received: 1 1 mycommand\n
   s>     {"action": "error", "message": "server already errored"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Missing flags on command name frame
 
@@ -588,11 +594,12 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 114\r\n
+  s>     Content-Length: 133\r\n
   s>     \r\n
   s>     received: 1 0 command1\n
   s>     {"action": "error", "message": "missing frame flags on command frame"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Missing argument frame
 
@@ -619,13 +626,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 197\r\n
+  s>     Content-Length: 216\r\n
   s>     \r\n
   s>     received: 1 2 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-args"}\n
   s>     received: 1 0 ignored\n
   s>     {"action": "error", "message": "expected command argument frame; got 1"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Argument frame with incomplete name
 
@@ -652,13 +660,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 206\r\n
+  s>     Content-Length: 225\r\n
   s>     \r\n
   s>     received: 1 2 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-args"}\n
   s>     received: 2 2 \x04\x00\xde\xadfoo\n
   s>     {"action": "error", "message": "malformed argument frame: partial argument name"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Argument frame with incomplete value
 
@@ -685,13 +694,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 219\r\n
+  s>     Content-Length: 238\r\n
   s>     \r\n
   s>     received: 1 2 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-args"}\n
   s>     received: 2 2 \x03\x00\xaa\xaafoopartialvalue\n
   s>     {"action": "error", "message": "malformed argument frame: partial argument value"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 Missing command data frame
 
@@ -718,13 +728,14 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 194\r\n
+  s>     Content-Length: 213\r\n
   s>     \r\n
   s>     received: 1 4 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-data"}\n
   s>     received: 1 1 command2\n
   s>     {"action": "error", "message": "expected command data frame; got 1"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
 No flags on command data frame
 
@@ -751,12 +762,13 @@ 
   s>     Server: testing stub value\r\n
   s>     Date: $HTTP_DATE$\r\n
   s>     Content-Type: text/plain\r\n
-  s>     Content-Length: 188\r\n
+  s>     Content-Length: 207\r\n
   s>     \r\n
   s>     received: 1 4 command1\n
   s>     {"action": "wantframe", "state": "command-receiving-data"}\n
   s>     received: 3 0 data\n
   s>     {"action": "error", "message": "command data frame without flags"}\n
-  s>     received: <no frame>
+  s>     received: <no frame>\n
+  s>     {"action": "noop"}
 
   $ cat error.log
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -401,6 +401,10 @@ 
         meta['action'] = action
         states.append(json.dumps(meta, sort_keys=True, separators=(', ', ': ')))
 
+    action, meta = reactor.oninputeof()
+    meta['action'] = action
+    states.append(json.dumps(meta, sort_keys=True, separators=(', ',': ')))
+
     res.status = b'200 OK'
     res.headers[b'Content-Type'] = b'text/plain'
     res.setbodybytes(b'\n'.join(states))
@@ -411,7 +415,11 @@ 
     Called when the HTTP request contains unified frame-based protocol
     frames for evaluation.
     """
-    reactor = wireprotoframing.serverreactor(ui, repo)
+    # TODO Some HTTP clients are full duplex and can receive data before
+    # the entire request is transmitted. Figure out a way to indicate support
+    # for that so we can opt into full duplex mode.
+    reactor = wireprotoframing.serverreactor(ui, repo,
+                                             deferoutput=True)
     seencommand = False
 
     while True:
@@ -448,6 +456,19 @@ 
             raise error.ProgrammingError(
                 'unhandled action from frame processor: %s' % action)
 
+    action, meta = reactor.oninputeof()
+    if action == 'sendframes':
+        # We assume we haven't started sending the response yet. If we're
+        # wrong, the response type will raise an exception.
+        res.status = b'200 OK'
+        res.headers[b'Content-Type'] = FRAMINGTYPE
+        res.setbodygen(meta['framegen'])
+    elif action == 'noop':
+        pass
+    else:
+        raise error.ProgrammingError('unhandled action from frame processor: %s'
+                                     % action)
+
 def _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand, reactor,
                       command):
     """Dispatch a wire protocol command made from HTTPv2 requests.
@@ -504,6 +525,8 @@ 
 
     if action == 'sendframes':
         res.setbodygen(meta['framegen'])
+    elif action == 'noop':
+        pass
     else:
         raise error.ProgrammingError('unhandled event from reactor: %s' %
                                      action)
diff --git a/mercurial/wireprotoframing.py b/mercurial/wireprotoframing.py
--- a/mercurial/wireprotoframing.py
+++ b/mercurial/wireprotoframing.py
@@ -269,12 +269,26 @@ 
     wantframe
        Indicates that nothing of interest happened and the server is waiting on
        more frames from the client before anything interesting can be done.
+
+    noop
+       Indicates no additional action is required.
     """
 
-    def __init__(self, ui, repo):
+    def __init__(self, ui, repo, deferoutput=False):
+        """Construct a new server reactor.
+
+        ``deferoutput`` can be used to indicate that no output frames should be
+        instructed to be sent until input has been exhausted. In this mode,
+        events that would normally generate output frames (such as a command
+        response being ready) will instead defer instructing the consumer to
+        send those frames. This is useful for half-duplex transports where the
+        sender cannot receive until all data has been transmitted.
+        """
         self._ui = ui
         self._repo = repo
+        self._deferoutput = deferoutput
         self._state = 'idle'
+        self._bufferedframegens = []
         self._activecommand = None
         self._activeargs = None
         self._activedata = None
@@ -307,8 +321,33 @@ 
 
         The raw bytes response is passed as an argument.
         """
+        framegen = createbytesresponseframesfrombytes(data)
+
+        if self._deferoutput:
+            self._bufferedframegens.append(framegen)
+            return 'noop', {}
+        else:
+            return 'sendframes', {
+                'framegen': framegen,
+            }
+
+    def oninputeof(self):
+        """Signals that end of input has been received.
+
+        No more frames will be received. All pending activity should be
+        completed.
+        """
+        if not self._deferoutput:
+            return 'noop', {}
+
+        # If we buffered all our responses, emit those.
+        def makegen():
+            for gen in self._bufferedframegens:
+                for frame in gen:
+                    yield frame
+
         return 'sendframes', {
-            'framegen': createbytesresponseframesfrombytes(data),
+            'framegen': makegen(),
         }
 
     def onapplicationerror(self, msg):