D3386: wireprotov2: change behavior of error frame
indygreg (Gregory Szorc)
phabricator at mercurial-scm.org
Sun Apr 15 00:22:45 UTC 2018
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
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``)
------------------------------------
To: indygreg, #hg-reviewers
Cc: mercurial-devel
More information about the Mercurial-devel
mailing list