Patchwork D3386: wireprotov2: change behavior of error frame

login
register
mail settings
Submitter phabricator
Date April 16, 2018, 11:10 p.m.
Message ID <3b2f1f5c41b0d9afe6c1d89a2dde0ff9@localhost.localdomain>
Download mbox | patch
Permalink /patch/31116/
State Not Applicable
Headers show

Comments

phabricator - April 16, 2018, 11:10 p.m.
This revision was automatically updated to reflect the committed changes.
Closed by commit rHG0c184ca594bb: wireprotov2: change behavior of error frame (authored by indygreg, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D3386?vs=8298&id=8329

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

AFFECTED FILES
  mercurial/help/internals/wireprotocol.txt
  mercurial/wireprotoframing.py
  mercurial/wireprotov2server.py
  tests/test-wireproto-serverreactor.py

CHANGE DETAILS




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

Patch

diff --git a/tests/test-wireproto-serverreactor.py b/tests/test-wireproto-serverreactor.py
--- a/tests/test-wireproto-serverreactor.py
+++ b/tests/test-wireproto-serverreactor.py
@@ -373,16 +373,18 @@ 
             b'1 2 0 command-response eos %s' % second,
         ])
 
-    def testapplicationerror(self):
+    def testservererror(self):
         reactor = makereactor()
         instream = framing.stream(1)
         list(sendcommandframes(reactor, instream, 1, b'mycommand', {}))
 
         outstream = reactor.makeoutputstream()
-        result = reactor.onapplicationerror(outstream, 1, b'some message')
+        result = reactor.onservererror(outstream, 1, b'some message')
         self.assertaction(result, b'sendframes')
         self.assertframesequal(result[1][b'framegen'], [
-            b'1 2 stream-begin error-response application some message',
+            b"1 2 stream-begin error-response 0 "
+            b"cbor:{b'type': b'server', "
+            b"b'message': [{b'msg': b'some message'}]}",
         ])
 
     def test1commanddeferresponse(self):
diff --git a/mercurial/wireprotov2server.py b/mercurial/wireprotov2server.py
--- a/mercurial/wireprotov2server.py
+++ b/mercurial/wireprotov2server.py
@@ -311,7 +311,7 @@ 
                                                       command['requestid'],
                                                       encoded)
     else:
-        action, meta = reactor.onapplicationerror(
+        action, meta = reactor.onservererror(
             _('unhandled response type from wire proto command'))
 
     if action == 'sendframes':
diff --git a/mercurial/wireprotoframing.py b/mercurial/wireprotoframing.py
--- a/mercurial/wireprotoframing.py
+++ b/mercurial/wireprotoframing.py
@@ -87,20 +87,12 @@ 
     b'eos': FLAG_COMMAND_RESPONSE_EOS,
 }
 
-FLAG_ERROR_RESPONSE_PROTOCOL = 0x01
-FLAG_ERROR_RESPONSE_APPLICATION = 0x02
-
-FLAGS_ERROR_RESPONSE = {
-    b'protocol': FLAG_ERROR_RESPONSE_PROTOCOL,
-    b'application': FLAG_ERROR_RESPONSE_APPLICATION,
-}
-
 # Maps frame types to their available flags.
 FRAME_TYPE_FLAGS = {
     FRAME_TYPE_COMMAND_REQUEST: FLAGS_COMMAND_REQUEST,
     FRAME_TYPE_COMMAND_DATA: FLAGS_COMMAND_DATA,
     FRAME_TYPE_COMMAND_RESPONSE: FLAGS_COMMAND_RESPONSE,
-    FRAME_TYPE_ERROR_RESPONSE: FLAGS_ERROR_RESPONSE,
+    FRAME_TYPE_ERROR_RESPONSE: {},
     FRAME_TYPE_TEXT_OUTPUT: {},
     FRAME_TYPE_PROGRESS: {},
     FRAME_TYPE_STREAM_SETTINGS: {},
@@ -394,20 +386,19 @@ 
         if done:
             break
 
-def createerrorframe(stream, requestid, msg, protocol=False, application=False):
+def createerrorframe(stream, requestid, msg, errtype):
     # TODO properly handle frame size limits.
     assert len(msg) <= DEFAULT_MAX_FRAME_SIZE
 
-    flags = 0
-    if protocol:
-        flags |= FLAG_ERROR_RESPONSE_PROTOCOL
-    if application:
-        flags |= FLAG_ERROR_RESPONSE_APPLICATION
+    payload = cbor.dumps({
+        b'type': errtype,
+        b'message': [{b'msg': msg}],
+    }, canonical=True)
 
     yield stream.makeframe(requestid=requestid,
                            typeid=FRAME_TYPE_ERROR_RESPONSE,
-                           flags=flags,
-                           payload=msg)
+                           flags=0,
+                           payload=payload)
 
 def createtextoutputframe(stream, requestid, atoms,
                           maxframesize=DEFAULT_MAX_FRAME_SIZE):
@@ -664,12 +655,12 @@ 
             'framegen': makegen(),
         }
 
-    def onapplicationerror(self, stream, requestid, msg):
+    def onservererror(self, stream, requestid, msg):
         ensureserverstream(stream)
 
         return 'sendframes', {
             'framegen': createerrorframe(stream, requestid, msg,
-                                         application=True),
+                                         errtype='server'),
         }
 
     def makeoutputstream(self):
@@ -1051,6 +1042,7 @@ 
 
         handlers = {
             FRAME_TYPE_COMMAND_RESPONSE: self._oncommandresponseframe,
+            FRAME_TYPE_ERROR_RESPONSE: self._onerrorresponseframe,
         }
 
         meth = handlers.get(frame.typeid)
@@ -1071,3 +1063,16 @@ 
             'eos': frame.flags & FLAG_COMMAND_RESPONSE_EOS,
             'data': frame.payload,
         }
+
+    def _onerrorresponseframe(self, request, frame):
+        request.state = 'errored'
+        del self._activerequests[request.requestid]
+
+        # The payload should be a CBOR map.
+        m = cbor.loads(frame.payload)
+
+        return 'error', {
+            'request': request,
+            'type': m['type'],
+            'message': m['message'],
+        }
diff --git a/mercurial/help/internals/wireprotocol.txt b/mercurial/help/internals/wireprotocol.txt
--- a/mercurial/help/internals/wireprotocol.txt
+++ b/mercurial/help/internals/wireprotocol.txt
@@ -688,23 +688,44 @@ 
 
 The ``0x01`` flag is mutually exclusive with the ``0x02`` flag.
 
-Error Response (``0x05``)
+Error Occurred (``0x05``)
 -------------------------
 
-An error occurred when processing a request. This could indicate
-a protocol-level failure or an application level failure depending
-on the flags for this message type.
+Some kind of error occurred.
+
+There are 3 general kinds of failures that can occur:
 
-The payload for this type is an error message that should be
-displayed to the user.
+* Command error encountered before any response issued
+* Command error encountered after a response was issued
+* Protocol or stream level error
+
+This frame type is used to capture the latter cases. (The general
+command error case is handled by the leading CBOR map in
+``Command Response`` frames.)
+
+The payload of this frame contains a CBOR map detailing the error. That
+map has the following bytestring keys:
 
-The following flag values are defined for this type:
+type
+   (bytestring) The overall type of error encountered. Can be one of the
+   following values:
+
+   protocol
+      A protocol-level error occurred. This typically means someone
+      is violating the framing protocol semantics and the server is
+      refusing to proceed.
 
-0x01
-   The error occurred at the transport/protocol level. If set, the
-   connection should be closed.
-0x02
-   The error occurred at the application level. e.g. invalid command.
+   server
+      A server-level error occurred. This typically indicates some kind of
+      logic error on the server, likely the fault of the server.
+
+   command
+      A command-level error, likely the fault of the client.
+
+message
+   (array of maps) A richly formatted message that is intended for
+   human consumption. See the ``Human Output Side-Channel`` frame
+   section for a description of the format of this data structure.
 
 Human Output Side-Channel (``0x06``)
 ------------------------------------