wireprotov2: change behavior of error frame

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...

Differential Revision: https://phab.mercurial-scm.org/D3386

mercurial/help/internals/wireprotocol.txt

@@ -688,23 +688,44 b' 0x02'
 
 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``)
 ------------------------------------

mercurial/wireprotoframing.py

@@ -87,20 +87,12 b' FLAGS_COMMAND_RESPONSE = {'
     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 b' def createcommandresponseframesfrombytes'
         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 b' class serverreactor(object):'
             '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 b' class clientreactor(object):'
 
         handlers = {
             FRAME_TYPE_COMMAND_RESPONSE: self._oncommandresponseframe,
+            FRAME_TYPE_ERROR_RESPONSE: self._onerrorresponseframe,
         }
 
         meth = handlers.get(frame.typeid)
@@ -1071,3 +1063,16 b' class clientreactor(object):'
             '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'],
+        }

mercurial/wireprotov2server.py

@@ -311,7 +311,7 b' def _httpv2runcommand(ui, repo, req, res'
                                                       command['requestid'],
                                                       encoded)
     else:
-        action, meta = reactor.onapplicationerror(
+        action, meta = reactor.onservererror(
             _('unhandled response type from wire proto command'))
 
     if action == 'sendframes':

tests/test-wireproto-serverreactor.py

@@ -373,16 +373,18 b' class ServerReactorTests(unittest.TestCa'
             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):