Mercurial > hg
changeset 37292:3d0e2cd86e05
wireproto: use CBOR for command requests
Now that we're using CBOR in the new wire protocol, let's convert
command requests to it.
Before I wrote this patch and was even thinking about CBOR, I was
thinking about how commands should be issued and came to the
conclusion that we didn't need separate frames to represent the
command name from its arguments. I already had a partially
completed patch prepared to merge the frames.
But with CBOR, it makes the implementation a bit simpler because
we don't need to roll our own serialization.
The changes here are a bit invasive. I tried to split this into
multiple commits to make it easier to review. But it was just too
hard.
* "command name" and "command argument" frames have been collapsed
into a "command request" frame.
* The flags for this new frame are totally different.
* Frame processing has been overhauled to reflect the new order
of things.
* Test fallout was significant. A handful of tests were removed.
Altogether, I think the new code is simpler. We don't have
complicated state around receiving commands. We're either receiving
command request frames or command data frames. We /could/
potentially collapse command data frames into command request
frames. Although I'd have to think a bit more about this before
I do it.
Differential Revision: https://phab.mercurial-scm.org/D2951
| author | Gregory Szorc <gregory.szorc@gmail.com> |
|---|---|
| date | Mon, 26 Mar 2018 14:34:32 -0700 |
| parents | b0041036214e |
| children | d5d665f6615a |
| files | mercurial/help/internals/wireprotocol.txt mercurial/wireprotoframing.py mercurial/wireprotoserver.py tests/test-http-api-httpv2.t tests/test-wireproto-serverreactor.py |
| diffstat | 5 files changed, 325 insertions(+), 361 deletions(-) [+] |
line wrap: on
line diff
--- a/mercurial/help/internals/wireprotocol.txt Mon Mar 26 10:50:36 2018 -0700 +++ b/mercurial/help/internals/wireprotocol.txt Mon Mar 26 14:34:32 2018 -0700 @@ -561,8 +561,17 @@ This frame contains a request to run a command. -The name of the command to run constitutes the entirety of the frame -payload. +The payload consists of a CBOR map defining the command request. The +bytestring keys of that map are: + +name + Name of the command that should be executed (bytestring). +args + Map of bytestring keys to various value types containing the named + arguments to this command. + + Each command defines its own set of argument names and their expected + types. This frame type MUST ONLY be sent from clients to servers: it is illegal for a server to send this frame to a client. @@ -570,54 +579,30 @@ The following flag values are defined for this type: 0x01 - End of command data. When set, the client will not send any command - arguments or additional command data. When set, the command has been - fully issued and the server has the full context to process the command. - The next frame issued by the client is not part of this command. + New command request. When set, this frame represents the beginning + of a new request to run a command. The ``Request ID`` attached to this + frame MUST NOT be active. 0x02 - Command argument frames expected. When set, the client will send - *Command Argument* frames containing command argument data. + Command request continuation. When set, this frame is a continuation + from a previous command request frame for its ``Request ID``. This + flag is set when the CBOR data for a command request does not fit + in a single frame. 0x04 - Command data frames expected. When set, the client will send - *Command Data* frames containing a raw stream of data for this - command. - -The ``0x01`` flag is mutually exclusive with both the ``0x02`` and ``0x04`` -flags. - -Command Argument (``0x02``) ---------------------------- - -This frame contains a named argument for a command. - -The frame type MUST ONLY be sent from clients to servers: it is illegal -for a server to send this frame to a client. + Additional frames expected. When set, the command request didn't fit + into a single frame and additional CBOR data follows in a subsequent + frame. +0x08 + Command data frames expected. When set, command data frames are + expected to follow the final command request frame for this request. -The payload consists of: - -* A 16-bit little endian integer denoting the length of the - argument name. -* A 16-bit little endian integer denoting the length of the - argument value. -* N bytes of ASCII data containing the argument name. -* N bytes of binary data containing the argument value. - -The payload MUST hold the entirety of the 32-bit header and the -argument name. The argument value MAY span multiple frames. If this -occurs, the appropriate frame flag should be set to indicate this. +``0x01`` MUST be set on the initial command request frame for a +``Request ID``. -The following flag values are defined for this type: +``0x01`` or ``0x02`` MUST be set to indicate this frame's role in +a series of command request frames. -0x01 - Argument data continuation. When set, the data for this argument did - not fit in a single frame and the next frame will contain additional - argument data. - -0x02 - End of arguments data. When set, the client will not send any more - command arguments for the command this frame is associated with. - The next frame issued by the client will be command data or - belong to a separate request. +If command data frames are to be sent, ``0x10`` MUST be set on ALL +command request frames. Command Data (``0x03``) ----------------------- @@ -903,8 +888,8 @@ A client can request that a remote run a command by sending it frames defining that command. This logical stream is composed of -1 ``Command Request`` frame, 0 or more ``Command Argument`` frames, -and 0 or more ``Command Data`` frames. +1 or more ``Command Request`` frames and and 0 or more ``Command Data`` +frames. All frames composing a single command request MUST be associated with the same ``Request ID``. @@ -928,14 +913,17 @@ TODO think about whether we should express dependencies between commands to avoid roundtrip latency. -Argument frames are the recommended mechanism for transferring fixed -sets of parameters to a command. Data frames are appropriate for -transferring variable data. A similar comparison would be to HTTP: -argument frames are headers and the message body is data frames. +A command is defined by a command name, 0 or more command arguments, +and optional command data. + +Arguments are the recommended mechanism for transferring fixed sets of +parameters to a command. Data is appropriate for transferring variable +data. Thinking in terms of HTTP, arguments would be headers and data +would be the message body. It is recommended for servers to delay the dispatch of a command -until all argument frames for that command have been received. Servers -MAY impose limits on the maximum argument size. +until all argument have been received. Servers MAY impose limits on the +maximum argument size. TODO define failure mechanism. Servers MAY dispatch to commands immediately once argument data
--- a/mercurial/wireprotoframing.py Mon Mar 26 10:50:36 2018 -0700 +++ b/mercurial/wireprotoframing.py Mon Mar 26 14:34:32 2018 -0700 @@ -39,8 +39,7 @@ b'encoded': STREAM_FLAG_ENCODING_APPLIED, } -FRAME_TYPE_COMMAND_NAME = 0x01 -FRAME_TYPE_COMMAND_ARGUMENT = 0x02 +FRAME_TYPE_COMMAND_REQUEST = 0x01 FRAME_TYPE_COMMAND_DATA = 0x03 FRAME_TYPE_BYTES_RESPONSE = 0x04 FRAME_TYPE_ERROR_RESPONSE = 0x05 @@ -49,8 +48,7 @@ FRAME_TYPE_STREAM_SETTINGS = 0x08 FRAME_TYPES = { - b'command-name': FRAME_TYPE_COMMAND_NAME, - b'command-argument': FRAME_TYPE_COMMAND_ARGUMENT, + b'command-request': FRAME_TYPE_COMMAND_REQUEST, b'command-data': FRAME_TYPE_COMMAND_DATA, b'bytes-response': FRAME_TYPE_BYTES_RESPONSE, b'error-response': FRAME_TYPE_ERROR_RESPONSE, @@ -59,22 +57,16 @@ b'stream-settings': FRAME_TYPE_STREAM_SETTINGS, } -FLAG_COMMAND_NAME_EOS = 0x01 -FLAG_COMMAND_NAME_HAVE_ARGS = 0x02 -FLAG_COMMAND_NAME_HAVE_DATA = 0x04 +FLAG_COMMAND_REQUEST_NEW = 0x01 +FLAG_COMMAND_REQUEST_CONTINUATION = 0x02 +FLAG_COMMAND_REQUEST_MORE_FRAMES = 0x04 +FLAG_COMMAND_REQUEST_EXPECT_DATA = 0x08 -FLAGS_COMMAND = { - b'eos': FLAG_COMMAND_NAME_EOS, - b'have-args': FLAG_COMMAND_NAME_HAVE_ARGS, - b'have-data': FLAG_COMMAND_NAME_HAVE_DATA, -} - -FLAG_COMMAND_ARGUMENT_CONTINUATION = 0x01 -FLAG_COMMAND_ARGUMENT_EOA = 0x02 - -FLAGS_COMMAND_ARGUMENT = { - b'continuation': FLAG_COMMAND_ARGUMENT_CONTINUATION, - b'eoa': FLAG_COMMAND_ARGUMENT_EOA, +FLAGS_COMMAND_REQUEST = { + b'new': FLAG_COMMAND_REQUEST_NEW, + b'continuation': FLAG_COMMAND_REQUEST_CONTINUATION, + b'more': FLAG_COMMAND_REQUEST_MORE_FRAMES, + b'have-data': FLAG_COMMAND_REQUEST_EXPECT_DATA, } FLAG_COMMAND_DATA_CONTINUATION = 0x01 @@ -103,8 +95,7 @@ # Maps frame types to their available flags. FRAME_TYPE_FLAGS = { - FRAME_TYPE_COMMAND_NAME: FLAGS_COMMAND, - FRAME_TYPE_COMMAND_ARGUMENT: FLAGS_COMMAND_ARGUMENT, + FRAME_TYPE_COMMAND_REQUEST: FLAGS_COMMAND_REQUEST, FRAME_TYPE_COMMAND_DATA: FLAGS_COMMAND_DATA, FRAME_TYPE_BYTES_RESPONSE: FLAGS_BYTES_RESPONSE, FRAME_TYPE_ERROR_RESPONSE: FLAGS_ERROR_RESPONSE, @@ -113,7 +104,7 @@ FRAME_TYPE_STREAM_SETTINGS: {}, } -ARGUMENT_FRAME_HEADER = struct.Struct(r'<HH') +ARGUMENT_RECORD_HEADER = struct.Struct(r'<HH') @attr.s(slots=True) class frameheader(object): @@ -269,43 +260,48 @@ return frame(h.requestid, h.streamid, h.streamflags, h.typeid, h.flags, payload) -def createcommandframes(stream, requestid, cmd, args, datafh=None): +def createcommandframes(stream, requestid, cmd, args, datafh=None, + maxframesize=DEFAULT_MAX_FRAME_SIZE): """Create frames necessary to transmit a request to run a command. This is a generator of bytearrays. Each item represents a frame ready to be sent over the wire to a peer. """ - flags = 0 + data = {b'name': cmd} if args: - flags |= FLAG_COMMAND_NAME_HAVE_ARGS - if datafh: - flags |= FLAG_COMMAND_NAME_HAVE_DATA + data[b'args'] = args - if not flags: - flags |= FLAG_COMMAND_NAME_EOS + data = cbor.dumps(data, canonical=True) - yield stream.makeframe(requestid=requestid, typeid=FRAME_TYPE_COMMAND_NAME, - flags=flags, payload=cmd) + offset = 0 + + while True: + flags = 0 - for i, k in enumerate(sorted(args)): - v = args[k] - last = i == len(args) - 1 + # Must set new or continuation flag. + if not offset: + flags |= FLAG_COMMAND_REQUEST_NEW + else: + flags |= FLAG_COMMAND_REQUEST_CONTINUATION - # TODO handle splitting of argument values across frames. - payload = bytearray(ARGUMENT_FRAME_HEADER.size + len(k) + len(v)) - offset = 0 - ARGUMENT_FRAME_HEADER.pack_into(payload, offset, len(k), len(v)) - offset += ARGUMENT_FRAME_HEADER.size - payload[offset:offset + len(k)] = k - offset += len(k) - payload[offset:offset + len(v)] = v + # Data frames is set on all frames. + if datafh: + flags |= FLAG_COMMAND_REQUEST_EXPECT_DATA - flags = FLAG_COMMAND_ARGUMENT_EOA if last else 0 + payload = data[offset:offset + maxframesize] + offset += len(payload) + + if len(payload) == maxframesize and offset < len(data): + flags |= FLAG_COMMAND_REQUEST_MORE_FRAMES + yield stream.makeframe(requestid=requestid, - typeid=FRAME_TYPE_COMMAND_ARGUMENT, + typeid=FRAME_TYPE_COMMAND_REQUEST, flags=flags, payload=payload) + if not (flags & FLAG_COMMAND_REQUEST_MORE_FRAMES): + break + if datafh: while True: data = datafh.read(DEFAULT_MAX_FRAME_SIZE) @@ -673,6 +669,12 @@ def _makeruncommandresult(self, requestid): entry = self._receivingcommands[requestid] + + if not entry['requestdone']: + self._state = 'errored' + raise error.ProgrammingError('should not be called without ' + 'requestdone set') + del self._receivingcommands[requestid] if self._receivingcommands: @@ -680,13 +682,25 @@ else: self._state = 'idle' + # Decode the payloads as CBOR. + entry['payload'].seek(0) + request = cbor.load(entry['payload']) + + if b'name' not in request: + self._state = 'errored' + return self._makeerrorresult( + _('command request missing "name" field')) + + if b'args' not in request: + request[b'args'] = {} + assert requestid not in self._activecommands self._activecommands.add(requestid) return 'runcommand', { 'requestid': requestid, - 'command': entry['command'], - 'args': entry['args'], + 'command': request[b'name'], + 'args': request[b'args'], 'data': entry['data'].getvalue() if entry['data'] else None, } @@ -695,13 +709,33 @@ 'state': self._state, } + def _validatecommandrequestframe(self, frame): + new = frame.flags & FLAG_COMMAND_REQUEST_NEW + continuation = frame.flags & FLAG_COMMAND_REQUEST_CONTINUATION + + if new and continuation: + self._state = 'errored' + return self._makeerrorresult( + _('received command request frame with both new and ' + 'continuation flags set')) + + if not new and not continuation: + self._state = 'errored' + return self._makeerrorresult( + _('received command request frame with neither new nor ' + 'continuation flags set')) + def _onframeidle(self, frame): # The only frame type that should be received in this state is a # command request. - if frame.typeid != FRAME_TYPE_COMMAND_NAME: + if frame.typeid != FRAME_TYPE_COMMAND_REQUEST: self._state = 'errored' return self._makeerrorresult( - _('expected command frame; got %d') % frame.typeid) + _('expected command request frame; got %d') % frame.typeid) + + res = self._validatecommandrequestframe(frame) + if res: + return res if frame.requestid in self._receivingcommands: self._state = 'errored' @@ -710,35 +744,45 @@ if frame.requestid in self._activecommands: self._state = 'errored' - return self._makeerrorresult(( - _('request with ID %d is already active') % frame.requestid)) + return self._makeerrorresult( + _('request with ID %d is already active') % frame.requestid) + + new = frame.flags & FLAG_COMMAND_REQUEST_NEW + moreframes = frame.flags & FLAG_COMMAND_REQUEST_MORE_FRAMES + expectingdata = frame.flags & FLAG_COMMAND_REQUEST_EXPECT_DATA - expectingargs = bool(frame.flags & FLAG_COMMAND_NAME_HAVE_ARGS) - expectingdata = bool(frame.flags & FLAG_COMMAND_NAME_HAVE_DATA) + if not new: + self._state = 'errored' + return self._makeerrorresult( + _('received command request frame without new flag set')) + + payload = util.bytesio() + payload.write(frame.payload) self._receivingcommands[frame.requestid] = { - 'command': frame.payload, - 'args': {}, + 'payload': payload, 'data': None, - 'expectingargs': expectingargs, - 'expectingdata': expectingdata, + 'requestdone': not moreframes, + 'expectingdata': bool(expectingdata), } - if frame.flags & FLAG_COMMAND_NAME_EOS: + # This is the final frame for this request. Dispatch it. + if not moreframes and not expectingdata: return self._makeruncommandresult(frame.requestid) - if expectingargs or expectingdata: - self._state = 'command-receiving' - return self._makewantframeresult() - else: - self._state = 'errored' - return self._makeerrorresult(_('missing frame flags on ' - 'command frame')) + assert moreframes or expectingdata + self._state = 'command-receiving' + return self._makewantframeresult() def _onframecommandreceiving(self, frame): - # It could be a new command request. Process it as such. - if frame.typeid == FRAME_TYPE_COMMAND_NAME: - return self._onframeidle(frame) + if frame.typeid == FRAME_TYPE_COMMAND_REQUEST: + # Process new command requests as such. + if frame.flags & FLAG_COMMAND_REQUEST_NEW: + return self._onframeidle(frame) + + res = self._validatecommandrequestframe(frame) + if res: + return res # All other frames should be related to a command that is currently # receiving but is not active. @@ -756,14 +800,30 @@ entry = self._receivingcommands[frame.requestid] - if frame.typeid == FRAME_TYPE_COMMAND_ARGUMENT: - if not entry['expectingargs']: + if frame.typeid == FRAME_TYPE_COMMAND_REQUEST: + moreframes = frame.flags & FLAG_COMMAND_REQUEST_MORE_FRAMES + expectingdata = bool(frame.flags & FLAG_COMMAND_REQUEST_EXPECT_DATA) + + if entry['requestdone']: + self._state = 'errored' + return self._makeerrorresult( + _('received command request frame when request frames ' + 'were supposedly done')) + + if expectingdata != entry['expectingdata']: self._state = 'errored' - return self._makeerrorresult(_( - 'received command argument frame for request that is not ' - 'expecting arguments: %d') % frame.requestid) + return self._makeerrorresult( + _('mismatch between expect data flag and previous frame')) + + entry['payload'].write(frame.payload) - return self._handlecommandargsframe(frame, entry) + if not moreframes: + entry['requestdone'] = True + + if not moreframes and not expectingdata: + return self._makeruncommandresult(frame.requestid) + + return self._makewantframeresult() elif frame.typeid == FRAME_TYPE_COMMAND_DATA: if not entry['expectingdata']: @@ -776,50 +836,10 @@ entry['data'] = util.bytesio() return self._handlecommanddataframe(frame, entry) - - def _handlecommandargsframe(self, frame, entry): - # The frame and state of command should have already been validated. - assert frame.typeid == FRAME_TYPE_COMMAND_ARGUMENT - - offset = 0 - namesize, valuesize = ARGUMENT_FRAME_HEADER.unpack_from(frame.payload) - offset += ARGUMENT_FRAME_HEADER.size - - # The argument name MUST fit inside the frame. - argname = bytes(frame.payload[offset:offset + namesize]) - offset += namesize - - if len(argname) != namesize: + else: self._state = 'errored' - return self._makeerrorresult(_('malformed argument frame: ' - 'partial argument name')) - - argvalue = bytes(frame.payload[offset:]) - - # Argument value spans multiple frames. Record our active state - # and wait for the next frame. - if frame.flags & FLAG_COMMAND_ARGUMENT_CONTINUATION: - raise error.ProgrammingError('not yet implemented') - - # Common case: the argument value is completely contained in this - # frame. - - if len(argvalue) != valuesize: - self._state = 'errored' - return self._makeerrorresult(_('malformed argument frame: ' - 'partial argument value')) - - entry['args'][argname] = argvalue - - if frame.flags & FLAG_COMMAND_ARGUMENT_EOA: - if entry['expectingdata']: - # TODO signal request to run a command once we don't - # buffer data frames. - return self._makewantframeresult() - else: - return self._makeruncommandresult(frame.requestid) - else: - return self._makewantframeresult() + return self._makeerrorresult(_( + 'received unexpected frame type: %d') % frame.typeid) def _handlecommanddataframe(self, frame, entry): assert frame.typeid == FRAME_TYPE_COMMAND_DATA
--- a/mercurial/wireprotoserver.py Mon Mar 26 10:50:36 2018 -0700 +++ b/mercurial/wireprotoserver.py Mon Mar 26 14:34:32 2018 -0700 @@ -36,7 +36,7 @@ HGTYPE = 'application/mercurial-0.1' HGTYPE2 = 'application/mercurial-0.2' HGERRTYPE = 'application/hg-error' -FRAMINGTYPE = b'application/mercurial-exp-framing-0002' +FRAMINGTYPE = b'application/mercurial-exp-framing-0003' HTTPV2 = wireprototypes.HTTPV2 SSHV1 = wireprototypes.SSHV1
--- a/tests/test-http-api-httpv2.t Mon Mar 26 10:50:36 2018 -0700 +++ b/tests/test-http-api-httpv2.t Mon Mar 26 14:34:32 2018 -0700 @@ -1,5 +1,5 @@ $ HTTPV2=exp-http-v2-0001 - $ MEDIATYPE=application/mercurial-exp-framing-0002 + $ MEDIATYPE=application/mercurial-exp-framing-0003 $ send() { > hg --verbose debugwireproto --peer raw http://$LOCALIP:$HGPORT/ @@ -122,7 +122,7 @@ s> Content-Type: text/plain\r\n s> Content-Length: 85\r\n s> \r\n - s> client MUST specify Accept header with value: application/mercurial-exp-framing-0002\n + s> client MUST specify Accept header with value: application/mercurial-exp-framing-0003\n Bad Accept header results in 406 @@ -145,7 +145,7 @@ s> Content-Type: text/plain\r\n s> Content-Length: 85\r\n s> \r\n - s> client MUST specify Accept header with value: application/mercurial-exp-framing-0002\n + s> client MUST specify Accept header with value: application/mercurial-exp-framing-0003\n Bad Content-Type header results in 415 @@ -158,7 +158,7 @@ using raw connection to peer s> POST /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n s> content-type: badmedia\r\n s> user-agent: test\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) @@ -170,7 +170,7 @@ s> Content-Type: text/plain\r\n s> Content-Length: 88\r\n s> \r\n - s> client MUST send Content-Type header with value: application/mercurial-exp-framing-0002\n + s> client MUST send Content-Type header with value: application/mercurial-exp-framing-0003\n Request to read-only command works out of the box @@ -179,23 +179,23 @@ > accept: $MEDIATYPE > content-type: $MEDIATYPE > user-agent: test - > frame 1 1 stream-begin command-name eos customreadonly + > frame 1 1 stream-begin command-request new cbor:{b'name': b'customreadonly'} > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> *\r\n (glob) + s> content-type: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n - s> *\r\n (glob) + s> content-length: 29\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x0e\x00\x00\x01\x00\x01\x01\x11customreadonly + s> \x15\x00\x00\x01\x00\x01\x01\x11\xa1DnameNcustomreadonly s> makefile('rb', None) s> HTTP/1.1 200 OK\r\n s> Server: testing stub value\r\n s> Date: $HTTP_DATE$\r\n - s> Content-Type: application/mercurial-exp-framing-0002\r\n + s> Content-Type: application/mercurial-exp-framing-0003\r\n s> Transfer-Encoding: chunked\r\n s> \r\n s> 25\r\n @@ -290,23 +290,23 @@ > user-agent: test > accept: $MEDIATYPE > content-type: $MEDIATYPE - > frame 1 1 stream-begin command-name eos customreadonly + > frame 1 1 stream-begin command-request new cbor:{b'name': b'customreadonly'} > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/rw/customreadonly HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n + s> content-type: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n - s> content-length: 22\r\n + s> content-length: 29\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x0e\x00\x00\x01\x00\x01\x01\x11customreadonly + s> \x15\x00\x00\x01\x00\x01\x01\x11\xa1DnameNcustomreadonly s> makefile('rb', None) s> HTTP/1.1 200 OK\r\n s> Server: testing stub value\r\n s> Date: $HTTP_DATE$\r\n - s> Content-Type: application/mercurial-exp-framing-0002\r\n + s> Content-Type: application/mercurial-exp-framing-0003\r\n s> Transfer-Encoding: chunked\r\n s> \r\n s> 25\r\n @@ -325,7 +325,7 @@ using raw connection to peer s> POST /api/exp-http-v2-0001/rw/badcommand HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n @@ -382,32 +382,26 @@ > accept: $MEDIATYPE > content-type: $MEDIATYPE > user-agent: test - > frame 1 1 stream-begin command-name have-args command1 - > frame 1 1 0 command-argument 0 \x03\x00\x04\x00fooval1 - > frame 1 1 0 command-argument eoa \x04\x00\x03\x00bar1val + > frame 1 1 stream-begin command-request new cbor:{b'name': b'command1', b'args': {b'foo': b'val1', b'bar1': b'val'}} > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/ro/debugreflect HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n + s> content-type: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n - s> content-length: 54\r\n + s> content-length: 47\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x08\x00\x00\x01\x00\x01\x01\x12command1\x0b\x00\x00\x01\x00\x01\x00 \x03\x00\x04\x00fooval1\x0b\x00\x00\x01\x00\x01\x00"\x04\x00\x03\x00bar1val + s> '\x00\x00\x01\x00\x01\x01\x11\xa2Dargs\xa2CfooDval1Dbar1CvalDnameHcommand1 s> makefile('rb', None) s> HTTP/1.1 200 OK\r\n s> Server: testing stub value\r\n s> Date: $HTTP_DATE$\r\n s> Content-Type: text/plain\r\n - s> Content-Length: 322\r\n + s> Content-Length: 205\r\n s> \r\n - s> received: 1 2 1 command1\n - s> ["wantframe", {"state": "command-receiving"}]\n - s> received: 2 0 1 \x03\x00\x04\x00fooval1\n - s> ["wantframe", {"state": "command-receiving"}]\n - s> received: 2 2 1 \x04\x00\x03\x00bar1val\n + s> received: 1 1 1 \xa2Dargs\xa2CfooDval1Dbar1CvalDnameHcommand1\n s> ["runcommand", {"args": {"bar1": "val", "foo": "val1"}, "command": "command1", "data": null, "requestid": 1}]\n s> received: <no frame>\n s> {"action": "noop"} @@ -419,27 +413,30 @@ > accept: $MEDIATYPE > content-type: $MEDIATYPE > user-agent: test - > frame 1 1 stream-begin command-name eos customreadonly - > frame 3 1 0 command-name eos customreadonly + > frame 1 1 stream-begin command-request new cbor:{b'name': b'customreadonly'} > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n + s> content-type: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n - s> content-length: 44\r\n + s> content-length: 29\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x0e\x00\x00\x01\x00\x01\x01\x11customreadonly\x0e\x00\x00\x03\x00\x01\x00\x11customreadonly + s> \x15\x00\x00\x01\x00\x01\x01\x11\xa1DnameNcustomreadonly s> makefile('rb', None) s> HTTP/1.1 200 OK\r\n s> Server: testing stub value\r\n s> Date: $HTTP_DATE$\r\n - s> Content-Type: text/plain\r\n - s> Content-Length: 46\r\n + s> Content-Type: application/mercurial-exp-framing-0003\r\n + s> Transfer-Encoding: chunked\r\n s> \r\n - s> multiple commands cannot be issued to this URL + s> 25\r\n + s> \x1d\x00\x00\x01\x00\x02\x01Bcustomreadonly bytes response + s> \r\n + s> 0\r\n + s> \r\n Multiple requests to "multirequest" URL are allowed @@ -448,27 +445,27 @@ > accept: $MEDIATYPE > content-type: $MEDIATYPE > user-agent: test - > frame 1 1 stream-begin command-name eos customreadonly - > frame 3 1 0 command-name eos customreadonly + > frame 1 1 stream-begin command-request new cbor:{b'name': b'customreadonly'} + > frame 3 1 0 command-request new cbor:{b'name': b'customreadonly'} > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> *\r\n (glob) + s> *\r\n (glob) s> user-agent: test\r\n - s> *\r\n (glob) + s> content-length: 58\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x0e\x00\x00\x01\x00\x01\x01\x11customreadonly\x0e\x00\x00\x03\x00\x01\x00\x11customreadonly + s> \x15\x00\x00\x01\x00\x01\x01\x11\xa1DnameNcustomreadonly\x15\x00\x00\x03\x00\x01\x00\x11\xa1DnameNcustomreadonly s> makefile('rb', None) s> HTTP/1.1 200 OK\r\n s> Server: testing stub value\r\n s> Date: $HTTP_DATE$\r\n - s> Content-Type: application/mercurial-exp-framing-0002\r\n + s> Content-Type: application/mercurial-exp-framing-0003\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> *\r\n (glob) + s> 25\r\n s> \x1d\x00\x00\x01\x00\x02\x01Bcustomreadonly bytes response s> \r\n s> 25\r\n @@ -484,36 +481,35 @@ > accept: $MEDIATYPE > content-type: $MEDIATYPE > user-agent: test - > frame 1 1 stream-begin command-name have-args listkeys - > frame 3 1 0 command-name have-args listkeys - > frame 3 1 0 command-argument eoa \x09\x00\x09\x00namespacebookmarks - > frame 1 1 0 command-argument eoa \x09\x00\x0a\x00namespacenamespaces + > frame 1 1 stream-begin command-request new|more \xa2Dargs\xa1Inamespace + > frame 3 1 0 command-request new|more \xa2Dargs\xa1Inamespace + > frame 3 1 0 command-request continuation JnamespacesDnameHlistkeys + > frame 1 1 0 command-request continuation IbookmarksDnameHlistkeys > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n + s> content-type: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n - s> content-length: 93\r\n + s> content-length: 115\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x08\x00\x00\x01\x00\x01\x01\x12listkeys\x08\x00\x00\x03\x00\x01\x00\x12listkeys\x16\x00\x00\x03\x00\x01\x00" \x00 \x00namespacebookmarks\x17\x00\x00\x01\x00\x01\x00" \x00\n - s> \x00namespacenamespaces + s> \x11\x00\x00\x01\x00\x01\x01\x15\xa2Dargs\xa1Inamespace\x11\x00\x00\x03\x00\x01\x00\x15\xa2Dargs\xa1Inamespace\x19\x00\x00\x03\x00\x01\x00\x12JnamespacesDnameHlistkeys\x18\x00\x00\x01\x00\x01\x00\x12IbookmarksDnameHlistkeys s> makefile('rb', None) s> HTTP/1.1 200 OK\r\n s> Server: testing stub value\r\n s> Date: $HTTP_DATE$\r\n - s> Content-Type: application/mercurial-exp-framing-0002\r\n + s> Content-Type: application/mercurial-exp-framing-0003\r\n s> Transfer-Encoding: chunked\r\n s> \r\n + s> 26\r\n + s> \x1e\x00\x00\x03\x00\x02\x01Bbookmarks \n + s> namespaces \n + s> phases + s> \r\n s> 8\r\n - s> \x00\x00\x00\x03\x00\x02\x01B - s> \r\n - s> 26\r\n - s> \x1e\x00\x00\x01\x00\x02\x00Bbookmarks \n - s> namespaces \n - s> phases + s> \x00\x00\x00\x01\x00\x02\x00B s> \r\n s> 0\r\n s> \r\n @@ -540,18 +536,18 @@ > accept: $MEDIATYPE > content-type: $MEDIATYPE > user-agent: test - > frame 1 1 stream-begin command-name eos unbundle + > frame 1 1 stream-begin command-request new cbor:{b'name': b'unbundle'} > EOF using raw connection to peer s> POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n s> Accept-Encoding: identity\r\n - s> accept: application/mercurial-exp-framing-0002\r\n - s> content-type: application/mercurial-exp-framing-0002\r\n + s> accept: application/mercurial-exp-framing-0003\r\n + s> content-type: application/mercurial-exp-framing-0003\r\n s> user-agent: test\r\n - s> content-length: 16\r\n + s> content-length: 23\r\n s> host: $LOCALIP:$HGPORT\r\n (glob) s> \r\n - s> \x08\x00\x00\x01\x00\x01\x01\x11unbundle + s> \x0f\x00\x00\x01\x00\x01\x01\x11\xa1DnameHunbundle s> makefile('rb', None) s> HTTP/1.1 403 Forbidden\r\n s> Server: testing stub value\r\n
--- a/tests/test-wireproto-serverreactor.py Mon Mar 26 10:50:36 2018 -0700 +++ b/tests/test-wireproto-serverreactor.py Mon Mar 26 14:34:32 2018 -0700 @@ -2,6 +2,9 @@ import unittest +from mercurial.thirdparty import ( + cbor, +) from mercurial import ( util, wireprotoframing as framing, @@ -96,7 +99,8 @@ frames = list(framing.createcommandframes(stream, 1, b'command', {}, data)) self.assertEqual(frames, [ - ffs(b'1 1 stream-begin command-name have-data command'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'command'}"), ffs(b'1 1 0 command-data continuation %s' % data.getvalue()), ffs(b'1 1 0 command-data eos ') ]) @@ -108,7 +112,8 @@ frames = list(framing.createcommandframes(stream, 1, b'command', {}, data)) self.assertEqual(frames, [ - ffs(b'1 1 stream-begin command-name have-data command'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'command'}"), ffs(b'1 1 0 command-data continuation %s' % ( b'x' * framing.DEFAULT_MAX_FRAME_SIZE)), ffs(b'1 1 0 command-data eos x'), @@ -125,10 +130,9 @@ }, data)) self.assertEqual(frames, [ - ffs(b'1 1 stream-begin command-name have-args|have-data command'), - ffs(br'1 1 0 command-argument 0 \x04\x00\x09\x00key1key1value'), - ffs(br'1 1 0 command-argument 0 \x04\x00\x09\x00key2key2value'), - ffs(br'1 1 0 command-argument eoa \x04\x00\x09\x00key3key3value'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'command', b'args': {b'key1': b'key1value', " + b"b'key2': b'key2value', b'key3': b'key3value'}}"), ffs(b'1 1 0 command-data eos %s' % data.getvalue()), ]) @@ -286,10 +290,9 @@ stream = framing.stream(1) results = list(sendcommandframes(reactor, stream, 41, b'mycommand', {b'foo': b'bar'})) - self.assertEqual(len(results), 2) - self.assertaction(results[0], 'wantframe') - self.assertaction(results[1], 'runcommand') - self.assertEqual(results[1][1], { + self.assertEqual(len(results), 1) + self.assertaction(results[0], 'runcommand') + self.assertEqual(results[0][1], { 'requestid': 41, 'command': b'mycommand', 'args': {b'foo': b'bar'}, @@ -301,11 +304,9 @@ stream = framing.stream(1) results = list(sendcommandframes(reactor, stream, 1, b'mycommand', {b'foo': b'bar', b'biz': b'baz'})) - self.assertEqual(len(results), 3) - self.assertaction(results[0], 'wantframe') - self.assertaction(results[1], 'wantframe') - self.assertaction(results[2], 'runcommand') - self.assertEqual(results[2][1], { + self.assertEqual(len(results), 1) + self.assertaction(results[0], 'runcommand') + self.assertEqual(results[0][1], { 'requestid': 1, 'command': b'mycommand', 'args': {b'foo': b'bar', b'biz': b'baz'}, @@ -329,7 +330,8 @@ def testmultipledataframes(self): frames = [ - ffs(b'1 1 stream-begin command-name have-data mycommand'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'mycommand'}"), ffs(b'1 1 0 command-data continuation data1'), ffs(b'1 1 0 command-data continuation data2'), ffs(b'1 1 0 command-data eos data3'), @@ -350,9 +352,9 @@ def testargumentanddata(self): frames = [ - ffs(b'1 1 stream-begin command-name have-args|have-data command'), - ffs(br'1 1 0 command-argument 0 \x03\x00\x03\x00keyval'), - ffs(br'1 1 0 command-argument eoa \x03\x00\x03\x00foobar'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'command', b'args': {b'key': b'val'," + b"b'foo': b'bar'}}"), ffs(b'1 1 0 command-data continuation value1'), ffs(b'1 1 0 command-data eos value2'), ] @@ -371,76 +373,68 @@ 'data': b'value1value2', }) - def testunexpectedcommandargument(self): - """Command argument frame when not running a command is an error.""" - result = self._sendsingleframe( - makereactor(), ffs(b'1 1 stream-begin command-argument 0 ignored')) + def testnewandcontinuation(self): + result = self._sendsingleframe(makereactor(), + ffs(b'1 1 stream-begin command-request new|continuation ')) self.assertaction(result, 'error') self.assertEqual(result[1], { - 'message': b'expected command frame; got 2', + 'message': b'received command request frame with both new and ' + b'continuation flags set', }) - def testunexpectedcommandargumentreceiving(self): - """Same as above but the command is receiving.""" - results = list(sendframes(makereactor(), [ - ffs(b'1 1 stream-begin command-name have-data command'), - ffs(b'1 1 0 command-argument eoa ignored'), - ])) - - self.assertaction(results[1], 'error') - self.assertEqual(results[1][1], { - 'message': b'received command argument frame for request that is ' - b'not expecting arguments: 1', + def testneithernewnorcontinuation(self): + result = self._sendsingleframe(makereactor(), + ffs(b'1 1 stream-begin command-request 0 ')) + self.assertaction(result, 'error') + self.assertEqual(result[1], { + 'message': b'received command request frame with neither new nor ' + b'continuation flags set', }) def testunexpectedcommanddata(self): - """Command argument frame when not running a command is an error.""" - result = self._sendsingleframe( - makereactor(), ffs(b'1 1 stream-begin command-data 0 ignored')) + """Command data frame when not running a command is an error.""" + result = self._sendsingleframe(makereactor(), + ffs(b'1 1 stream-begin command-data 0 ignored')) self.assertaction(result, 'error') self.assertEqual(result[1], { - 'message': b'expected command frame; got 3', + 'message': b'expected command request frame; got 3', }) def testunexpectedcommanddatareceiving(self): """Same as above except the command is receiving.""" results = list(sendframes(makereactor(), [ - ffs(b'1 1 stream-begin command-name have-args command'), + ffs(b'1 1 stream-begin command-request new|more ' + b"cbor:{b'name': b'ignored'}"), ffs(b'1 1 0 command-data eos ignored'), ])) + self.assertaction(results[0], 'wantframe') self.assertaction(results[1], 'error') self.assertEqual(results[1][1], { 'message': b'received command data frame for request that is not ' b'expecting data: 1', }) - def testmissingcommandframeflags(self): - """Command name frame must have flags set.""" - result = self._sendsingleframe( - makereactor(), ffs(b'1 1 stream-begin command-name 0 command')) - self.assertaction(result, 'error') - self.assertEqual(result[1], { - 'message': b'missing frame flags on command frame', - }) - def testconflictingrequestidallowed(self): """Multiple fully serviced commands with same request ID is allowed.""" reactor = makereactor() results = [] outstream = reactor.makeoutputstream() results.append(self._sendsingleframe( - reactor, ffs(b'1 1 stream-begin command-name eos command'))) + reactor, ffs(b'1 1 stream-begin command-request new ' + b"cbor:{b'name': b'command'}"))) result = reactor.onbytesresponseready(outstream, 1, b'response1') self.assertaction(result, 'sendframes') list(result[1]['framegen']) results.append(self._sendsingleframe( - reactor, ffs(b'1 1 0 command-name eos command'))) + reactor, ffs(b'1 1 stream-begin command-request new ' + b"cbor:{b'name': b'command'}"))) result = reactor.onbytesresponseready(outstream, 1, b'response2') self.assertaction(result, 'sendframes') list(result[1]['framegen']) results.append(self._sendsingleframe( - reactor, ffs(b'1 1 0 command-name eos command'))) + reactor, ffs(b'1 1 stream-begin command-request new ' + b"cbor:{b'name': b'command'}"))) result = reactor.onbytesresponseready(outstream, 1, b'response3') self.assertaction(result, 'sendframes') list(result[1]['framegen']) @@ -457,8 +451,10 @@ def testconflictingrequestid(self): """Request ID for new command matching in-flight command is illegal.""" results = list(sendframes(makereactor(), [ - ffs(b'1 1 stream-begin command-name have-args command'), - ffs(b'1 1 0 command-name eos command'), + ffs(b'1 1 stream-begin command-request new|more ' + b"cbor:{b'name': b'command'}"), + ffs(b'1 1 0 command-request new ' + b"cbor:{b'name': b'command1'}"), ])) self.assertaction(results[0], 'wantframe') @@ -468,13 +464,28 @@ }) def testinterleavedcommands(self): + cbor1 = cbor.dumps({ + b'name': b'command1', + b'args': { + b'foo': b'bar', + b'key1': b'val', + } + }, canonical=True) + cbor3 = cbor.dumps({ + b'name': b'command3', + b'args': { + b'biz': b'baz', + b'key': b'val', + }, + }, canonical=True) + results = list(sendframes(makereactor(), [ - ffs(b'1 1 stream-begin command-name have-args command1'), - ffs(b'3 1 0 command-name have-args command3'), - ffs(br'1 1 0 command-argument 0 \x03\x00\x03\x00foobar'), - ffs(br'3 1 0 command-argument 0 \x03\x00\x03\x00bizbaz'), - ffs(br'3 1 0 command-argument eoa \x03\x00\x03\x00keyval'), - ffs(br'1 1 0 command-argument eoa \x04\x00\x03\x00key1val'), + ffs(b'1 1 stream-begin command-request new|more %s' % cbor1[0:6]), + ffs(b'3 1 0 command-request new|more %s' % cbor3[0:10]), + ffs(b'1 1 0 command-request continuation|more %s' % cbor1[6:9]), + ffs(b'3 1 0 command-request continuation|more %s' % cbor3[10:13]), + ffs(b'3 1 0 command-request continuation %s' % cbor3[13:]), + ffs(b'1 1 0 command-request continuation %s' % cbor1[9:]), ])) self.assertEqual([t[0] for t in results], [ @@ -499,53 +510,14 @@ 'data': None, }) - def testmissingargumentframe(self): - # This test attempts to test behavior when reactor has an incomplete - # command request waiting on argument data. But it doesn't handle that - # scenario yet. So this test does nothing of value. - frames = [ - ffs(b'1 1 stream-begin command-name have-args command'), - ] - - results = list(sendframes(makereactor(), frames)) - self.assertaction(results[0], 'wantframe') - - def testincompleteargumentname(self): - """Argument frame with incomplete name.""" - frames = [ - ffs(b'1 1 stream-begin command-name have-args command1'), - ffs(br'1 1 0 command-argument eoa \x04\x00\xde\xadfoo'), - ] - - results = list(sendframes(makereactor(), frames)) - self.assertEqual(len(results), 2) - self.assertaction(results[0], 'wantframe') - self.assertaction(results[1], 'error') - self.assertEqual(results[1][1], { - 'message': b'malformed argument frame: partial argument name', - }) - - def testincompleteargumentvalue(self): - """Argument frame with incomplete value.""" - frames = [ - ffs(b'1 1 stream-begin command-name have-args command'), - ffs(br'1 1 0 command-argument eoa \x03\x00\xaa\xaafoopartialvalue'), - ] - - results = list(sendframes(makereactor(), frames)) - self.assertEqual(len(results), 2) - self.assertaction(results[0], 'wantframe') - self.assertaction(results[1], 'error') - self.assertEqual(results[1][1], { - 'message': b'malformed argument frame: partial argument value', - }) - def testmissingcommanddataframe(self): # The reactor doesn't currently handle partially received commands. # So this test is failing to do anything with request 1. frames = [ - ffs(b'1 1 stream-begin command-name have-data command1'), - ffs(b'3 1 0 command-name eos command2'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'command1'}"), + ffs(b'3 1 0 command-request new ' + b"cbor:{b'name': b'command2'}"), ] results = list(sendframes(makereactor(), frames)) self.assertEqual(len(results), 2) @@ -554,7 +526,8 @@ def testmissingcommanddataframeflags(self): frames = [ - ffs(b'1 1 stream-begin command-name have-data command1'), + ffs(b'1 1 stream-begin command-request new|have-data ' + b"cbor:{b'name': b'command1'}"), ffs(b'1 1 0 command-data 0 data'), ] results = list(sendframes(makereactor(), frames)) @@ -568,9 +541,11 @@ def testframefornonreceivingrequest(self): """Receiving a frame for a command that is not receiving is illegal.""" results = list(sendframes(makereactor(), [ - ffs(b'1 1 stream-begin command-name eos command1'), - ffs(b'3 1 0 command-name have-data command3'), - ffs(b'5 1 0 command-argument eoa ignored'), + ffs(b'1 1 stream-begin command-request new ' + b"cbor:{b'name': b'command1'}"), + ffs(b'3 1 0 command-request new|have-data ' + b"cbor:{b'name': b'command3'}"), + ffs(b'5 1 0 command-data eos ignored'), ])) self.assertaction(results[2], 'error') self.assertEqual(results[2][1], { @@ -705,21 +680,6 @@ 'message': b'request with ID 1 is already active', }) - def testduplicaterequestargumentframe(self): - """Variant on above except we sent an argument frame instead of name.""" - reactor = makereactor() - stream = framing.stream(1) - list(sendcommandframes(reactor, stream, 1, b'command', {})) - results = list(sendframes(reactor, [ - ffs(b'3 1 stream-begin command-name have-args command'), - ffs(b'1 1 0 command-argument 0 ignored'), - ])) - self.assertaction(results[0], 'wantframe') - self.assertaction(results[1], 'error') - self.assertEqual(results[1][1], { - 'message': 'received frame for request that is still active: 1', - }) - def testduplicaterequestaftersend(self): """We can use a duplicate request ID after we've sent the response.""" reactor = makereactor()