Patchwork D3386: wireprotov2: change behavior of error frame

login
register
mail settings
Submitter phabricator
Date April 15, 2018, 12:22 a.m.
Message ID <differential-rev-PHID-DREV-zt3xme5mjqav3srm7c75-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31066/
State Superseded
Headers show

Comments

phabricator - April 15, 2018, 12:22 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Now that we have a leading CBOR map in command response frames
  to indicate overall command result status, we don't need to use
  the error response frame to represent command errors. Instead,
  we can reserve it for protocol and server level errors. And for the
  special case of a command error that occurred after command response
  frames were emitted.
  
  The code for error handling still needs a ton of work. But we're
  slowly going in the right direction...

REPOSITORY
  rHG Mercurial

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
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``)
 ------------------------------------