This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3386

wireprotov2: change behavior of error frame
ClosedPublic

Authored by indygreg on Apr 14 2018, 8:21 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG0c184ca594bb: wireprotov2: change behavior of error frame
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...

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Apr 14 2018, 8:21 PM
durin42 accepted this revision.Apr 16 2018, 6:52 PM
This revision is now accepted and ready to land.Apr 16 2018, 6:52 PM
Closed by commit rHG0c184ca594bb: wireprotov2: change behavior of error frame (authored by indygreg). · Explain WhyApr 16 2018, 7:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 8329

mercurial/help/internals/wireprotocol.txt

Data continuation. When set, an additional frame containing response data Data continuation. When set, an additional frame containing response data
will follow. will follow.
0x02 0x02
End of data. When set, the response data has been fully sent and End of data. When set, the response data has been fully sent and
no additional frames for this response will be sent. no additional frames for this response will be sent.
The ``0x01`` flag is mutually exclusive with the ``0x02`` flag. 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 Some kind of error occurred.
a protocol-level failure or an application level failure depending
on the flags for this message type.
The payload for this type is an error message that should be There are 3 general kinds of failures that can occur:
displayed to the user.
The following flag values are defined for this type: * Command error encountered before any response issued
* Command error encountered after a response was issued
* Protocol or stream level error
0x01 This frame type is used to capture the latter cases. (The general
The error occurred at the transport/protocol level. If set, the command error case is handled by the leading CBOR map in
connection should be closed. ``Command Response`` frames.)
0x02
The error occurred at the application level. e.g. invalid command. The payload of this frame contains a CBOR map detailing the error. That
map has the following bytestring keys:
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.
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``) Human Output Side-Channel (``0x06``)
------------------------------------ ------------------------------------
This frame contains a message that is intended to be displayed to This frame contains a message that is intended to be displayed to
people. Whereas most frames communicate machine readable data, this people. Whereas most frames communicate machine readable data, this
frame communicates textual data that is intended to be shown to frame communicates textual data that is intended to be shown to
humans. humans.

mercurial/wireprotoframing.py

FLAG_COMMAND_RESPONSE_CONTINUATION = 0x01 FLAG_COMMAND_RESPONSE_CONTINUATION = 0x01
FLAG_COMMAND_RESPONSE_EOS = 0x02 FLAG_COMMAND_RESPONSE_EOS = 0x02
FLAGS_COMMAND_RESPONSE = { FLAGS_COMMAND_RESPONSE = {
b'continuation': FLAG_COMMAND_RESPONSE_CONTINUATION, b'continuation': FLAG_COMMAND_RESPONSE_CONTINUATION,
b'eos': FLAG_COMMAND_RESPONSE_EOS, 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. # Maps frame types to their available flags.
FRAME_TYPE_FLAGS = { FRAME_TYPE_FLAGS = {
FRAME_TYPE_COMMAND_REQUEST: FLAGS_COMMAND_REQUEST, FRAME_TYPE_COMMAND_REQUEST: FLAGS_COMMAND_REQUEST,
FRAME_TYPE_COMMAND_DATA: FLAGS_COMMAND_DATA, FRAME_TYPE_COMMAND_DATA: FLAGS_COMMAND_DATA,
FRAME_TYPE_COMMAND_RESPONSE: FLAGS_COMMAND_RESPONSE, FRAME_TYPE_COMMAND_RESPONSE: FLAGS_COMMAND_RESPONSE,
FRAME_TYPE_ERROR_RESPONSE: FLAGS_ERROR_RESPONSE, FRAME_TYPE_ERROR_RESPONSE: {},
FRAME_TYPE_TEXT_OUTPUT: {}, FRAME_TYPE_TEXT_OUTPUT: {},
FRAME_TYPE_PROGRESS: {}, FRAME_TYPE_PROGRESS: {},
FRAME_TYPE_STREAM_SETTINGS: {}, FRAME_TYPE_STREAM_SETTINGS: {},
} }
ARGUMENT_RECORD_HEADER = struct.Struct(r'<HH') ARGUMENT_RECORD_HEADER = struct.Struct(r'<HH')
def humanflags(mapping, value): def humanflags(mapping, value):
yield stream.makeframe(requestid=requestid, yield stream.makeframe(requestid=requestid,
typeid=FRAME_TYPE_COMMAND_RESPONSE, typeid=FRAME_TYPE_COMMAND_RESPONSE,
flags=flags, flags=flags,
payload=chunk) payload=chunk)
if done: if done:
break break
def createerrorframe(stream, requestid, msg, protocol=False, application=False): def createerrorframe(stream, requestid, msg, errtype):
# TODO properly handle frame size limits. # TODO properly handle frame size limits.
assert len(msg) <= DEFAULT_MAX_FRAME_SIZE assert len(msg) <= DEFAULT_MAX_FRAME_SIZE
flags = 0 payload = cbor.dumps({
if protocol: b'type': errtype,
flags |= FLAG_ERROR_RESPONSE_PROTOCOL b'message': [{b'msg': msg}],
if application: }, canonical=True)
flags |= FLAG_ERROR_RESPONSE_APPLICATION
yield stream.makeframe(requestid=requestid, yield stream.makeframe(requestid=requestid,
typeid=FRAME_TYPE_ERROR_RESPONSE, typeid=FRAME_TYPE_ERROR_RESPONSE,
flags=flags, flags=0,
payload=msg) payload=payload)
def createtextoutputframe(stream, requestid, atoms, def createtextoutputframe(stream, requestid, atoms,
maxframesize=DEFAULT_MAX_FRAME_SIZE): maxframesize=DEFAULT_MAX_FRAME_SIZE):
"""Create a text output frame to render text to people. """Create a text output frame to render text to people.
``atoms`` is a 3-tuple of (formatting string, args, labels). ``atoms`` is a 3-tuple of (formatting string, args, labels).
The formatting string contains ``%s`` tokens to be replaced by the The formatting string contains ``%s`` tokens to be replaced by the
for gen in self._bufferedframegens: for gen in self._bufferedframegens:
for frame in gen: for frame in gen:
yield frame yield frame
return 'sendframes', { return 'sendframes', {
'framegen': makegen(), 'framegen': makegen(),
} }
def onapplicationerror(self, stream, requestid, msg): def onservererror(self, stream, requestid, msg):
ensureserverstream(stream) ensureserverstream(stream)
return 'sendframes', { return 'sendframes', {
'framegen': createerrorframe(stream, requestid, msg, 'framegen': createerrorframe(stream, requestid, msg,
application=True), errtype='server'),
} }
def makeoutputstream(self): def makeoutputstream(self):
"""Create a stream to be used for sending data to the client.""" """Create a stream to be used for sending data to the client."""
streamid = self._nextoutgoingstreamid streamid = self._nextoutgoingstreamid
self._nextoutgoingstreamid += 2 self._nextoutgoingstreamid += 2
s = stream(streamid) s = stream(streamid)
frame.requestid), frame.requestid),
} }
request = self._activerequests[frame.requestid] request = self._activerequests[frame.requestid]
request.state = 'receiving' request.state = 'receiving'
handlers = { handlers = {
FRAME_TYPE_COMMAND_RESPONSE: self._oncommandresponseframe, FRAME_TYPE_COMMAND_RESPONSE: self._oncommandresponseframe,
FRAME_TYPE_ERROR_RESPONSE: self._onerrorresponseframe,
} }
meth = handlers.get(frame.typeid) meth = handlers.get(frame.typeid)
if not meth: if not meth:
raise error.ProgrammingError('unhandled frame type: %d' % raise error.ProgrammingError('unhandled frame type: %d' %
frame.typeid) frame.typeid)
return meth(request, frame) return meth(request, frame)
def _oncommandresponseframe(self, request, frame): def _oncommandresponseframe(self, request, frame):
if frame.flags & FLAG_COMMAND_RESPONSE_EOS: if frame.flags & FLAG_COMMAND_RESPONSE_EOS:
request.state = 'received' request.state = 'received'
del self._activerequests[request.requestid] del self._activerequests[request.requestid]
return 'responsedata', { return 'responsedata', {
'request': request, 'request': request,
'expectmore': frame.flags & FLAG_COMMAND_RESPONSE_CONTINUATION, 'expectmore': frame.flags & FLAG_COMMAND_RESPONSE_CONTINUATION,
'eos': frame.flags & FLAG_COMMAND_RESPONSE_EOS, 'eos': frame.flags & FLAG_COMMAND_RESPONSE_EOS,
'data': frame.payload, '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

command['requestid'], command['requestid'],
rsp.data) rsp.data)
elif isinstance(rsp, wireprototypes.cborresponse): elif isinstance(rsp, wireprototypes.cborresponse):
encoded = cbor.dumps(rsp.value, canonical=True) encoded = cbor.dumps(rsp.value, canonical=True)
action, meta = reactor.oncommandresponseready(outstream, action, meta = reactor.oncommandresponseready(outstream,
command['requestid'], command['requestid'],
encoded) encoded)
else: else:
action, meta = reactor.onapplicationerror( action, meta = reactor.onservererror(
_('unhandled response type from wire proto command')) _('unhandled response type from wire proto command'))
if action == 'sendframes': if action == 'sendframes':
res.setbodygen(meta['framegen']) res.setbodygen(meta['framegen'])
return True return True
elif action == 'noop': elif action == 'noop':
return False return False
else: else:

tests/test-wireproto-serverreactor.py

result = reactor.oncommandresponseready(outstream, 1, first + second) result = reactor.oncommandresponseready(outstream, 1, first + second)
self.assertaction(result, b'sendframes') self.assertaction(result, b'sendframes')
self.assertframesequal(result[1][b'framegen'], [ self.assertframesequal(result[1][b'framegen'], [
b'1 2 stream-begin command-response continuation %s' % OK, b'1 2 stream-begin command-response continuation %s' % OK,
b'1 2 0 command-response continuation %s' % first, b'1 2 0 command-response continuation %s' % first,
b'1 2 0 command-response eos %s' % second, b'1 2 0 command-response eos %s' % second,
]) ])
def testapplicationerror(self): def testservererror(self):
reactor = makereactor() reactor = makereactor()
instream = framing.stream(1) instream = framing.stream(1)
list(sendcommandframes(reactor, instream, 1, b'mycommand', {})) list(sendcommandframes(reactor, instream, 1, b'mycommand', {}))
outstream = reactor.makeoutputstream() 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.assertaction(result, b'sendframes')
self.assertframesequal(result[1][b'framegen'], [ 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): def test1commanddeferresponse(self):
"""Responses when in deferred output mode are delayed until EOF.""" """Responses when in deferred output mode are delayed until EOF."""
reactor = makereactor(deferoutput=True) reactor = makereactor(deferoutput=True)
instream = framing.stream(1) instream = framing.stream(1)
results = list(sendcommandframes(reactor, instream, 1, b'mycommand', results = list(sendcommandframes(reactor, instream, 1, b'mycommand',
{})) {}))