This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4474

wireprotov2peer: stream decoded responses
ClosedPublic

Authored by indygreg on Sep 4 2018, 2:28 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGd06834e0f48e: wireprotov2peer: stream decoded responses
Summary

Previously, wire protocol version 2 would buffer all response data.
Only once all data was received did we CBOR decode it and resolve
the future associated with the command. This was obviously not
desirable. In future commits that introduce large response payloads,
this caused significant memory bloat and slowed down client
operations due to waiting on the server.

This commit refactors the response handling code so that response
data can be streamed.

Command response objects now contain a buffered CBOR decoder. As
new data arrives, it is fed into the decoder. Decoded objects are
made available to the generator as they are decoded.

Because there is a separate thread processing incoming frames and
feeding data into the response object, there is the potential for
race conditions when mutating response objects. So a lock has been
added to guard access to critical state variables.

Because the generator emitting decoded objects needs to wait on
those objects to become available, we've added an Event for the
generator to wait on so it doesn't busy loop. This does mean
there is the potential for deadlocks. And I'm pretty sure they can
occur in some scenarios. We already have a handful of TODOs around
this. But I've added some more. Fixing this will likely require
moving the background thread receiving frames into clienthandler.
We likely would have done this anyway when implementing the client
bits for the SSH transport.

Test output changes because the initial CBOR map holding the overall
response state is now always handled internally by the response
object.

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.

Revision Contents
Changeset List

Diff 10935

mercurial/debugcommands.py

ui.status(_('result: %s\n') % stringutil.escapestr(res)) ui.status(_('result: %s\n') % stringutil.escapestr(res))
ui.status(_('remote output: %s\n') % ui.status(_('remote output: %s\n') %
stringutil.escapestr(output)) stringutil.escapestr(output))
else: else:
with peer.commandexecutor() as e: with peer.commandexecutor() as e:
res = e.callcommand(command, args).result() res = e.callcommand(command, args).result()
if isinstance(res, wireprotov2peer.commandresponse): if isinstance(res, wireprotov2peer.commandresponse):
val = list(res.cborobjects()) val = res.objects()
ui.status(_('response: %s\n') % ui.status(_('response: %s\n') %
stringutil.pprint(val, bprefix=True, indent=2)) stringutil.pprint(val, bprefix=True, indent=2))
else: else:
ui.status(_('response: %s\n') % ui.status(_('response: %s\n') %
stringutil.pprint(res, bprefix=True, indent=2)) stringutil.pprint(res, bprefix=True, indent=2))
elif action == 'batchbegin': elif action == 'batchbegin':
if batchedcommands is not None: if batchedcommands is not None:

mercurial/wireprotov2peer.py

# wireprotov2peer.py - client side code for wire protocol version 2 # wireprotov2peer.py - client side code for wire protocol version 2
# #
# Copyright 2018 Gregory Szorc <gregory.szorc@gmail.com> # Copyright 2018 Gregory Szorc <gregory.szorc@gmail.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import threading
from .i18n import _ from .i18n import _
from . import ( from . import (
encoding, encoding,
error, error,
util,
wireprotoframing, wireprotoframing,
) )
from .utils import ( from .utils import (
cborutil, cborutil,
) )
def formatrichmessage(atoms): def formatrichmessage(atoms):
"""Format an encoded message from the framing protocol.""" """Format an encoded message from the framing protocol."""
chunks = [] chunks = []
for atom in atoms: for atom in atoms:
msg = _(atom[b'msg']) msg = _(atom[b'msg'])
if b'args' in atom: if b'args' in atom:
msg = msg % tuple(atom[b'args']) msg = msg % tuple(atom[b'args'])
chunks.append(msg) chunks.append(msg)
return b''.join(chunks) return b''.join(chunks)
class commandresponse(object): class commandresponse(object):
"""Represents the response to a command request.""" """Represents the response to a command request.
Instances track the state of the command and hold its results.
An external entity is required to update the state of the object when
events occur.
"""
def __init__(self, requestid, command): def __init__(self, requestid, command):
self.requestid = requestid self.requestid = requestid
self.command = command self.command = command
self.b = util.bytesio() # Whether all remote input related to this command has been
# received.
self._inputcomplete = False
# We have a lock that is acquired when important object state is
# mutated. This is to prevent race conditions between 1 thread
# sending us new data and another consuming it.
self._lock = threading.RLock()
# An event is set when state of the object changes. This event
# is waited on by the generator emitting objects.
self._serviceable = threading.Event()
self._pendingevents = []
self._decoder = cborutil.bufferingdecoder()
self._seeninitial = False
def _oninputcomplete(self):
with self._lock:
self._inputcomplete = True
self._serviceable.set()
def _onresponsedata(self, data):
available, readcount, wanted = self._decoder.decode(data)
if not available:
return
with self._lock:
for o in self._decoder.getavailable():
if not self._seeninitial:
self._handleinitial(o)
continue
self._pendingevents.append(o)
self._serviceable.set()
def _handleinitial(self, o):
self._seeninitial = True
if o[b'status'] == 'ok':
return
atoms = [{'msg': o[b'error'][b'message']}]
if b'args' in o[b'error']:
atoms[0]['args'] = o[b'error'][b'args']
raise error.RepoError(formatrichmessage(atoms))
def cborobjects(self): def objects(self):
"""Obtain decoded CBOR objects from this response.""" """Obtained decoded objects from this response.
self.b.seek(0)
for v in cborutil.decodeall(self.b.getvalue()): This is a generator of data structures that were decoded from the
yield v command response.
Obtaining the next member of the generator may block due to waiting
on external data to become available.
If the server encountered an error in the middle of serving the data
or if another error occurred, an exception may be raised when
advancing the generator.
"""
while True:
# TODO this can infinite loop if self._inputcomplete is never
# set. We likely want to tie the lifetime of this object/state
# to that of the background thread receiving frames and updating
# our state.
self._serviceable.wait(1.0)
with self._lock:
self._serviceable.clear()
# Make copies because objects could be mutated during
# iteration.
stop = self._inputcomplete
pending = list(self._pendingevents)
self._pendingevents[:] = []
for o in pending:
yield o
if stop:
break
class clienthandler(object): class clienthandler(object):
"""Object to handle higher-level client activities. """Object to handle higher-level client activities.
The ``clientreactor`` is used to hold low-level state about the frame-based The ``clientreactor`` is used to hold low-level state about the frame-based
protocol, such as which requests and streams are active. This type is used protocol, such as which requests and streams are active. This type is used
for higher-level operations, such as reading frames from a socket, exposing for higher-level operations, such as reading frames from a socket, exposing
and managing a higher-level primitive for representing command responses, and managing a higher-level primitive for representing command responses,
request, action, meta = self._reactor.callcommand(command, args) request, action, meta = self._reactor.callcommand(command, args)
if action != 'noop': if action != 'noop':
raise error.ProgrammingError('%s not yet supported' % action) raise error.ProgrammingError('%s not yet supported' % action)
rid = request.requestid rid = request.requestid
self._requests[rid] = request self._requests[rid] = request
self._futures[rid] = f self._futures[rid] = f
# TODO we need some kind of lifetime on response instances otherwise
# objects() may deadlock.
self._responses[rid] = commandresponse(rid, command) self._responses[rid] = commandresponse(rid, command)
return iter(()) return iter(())
def flushcommands(self): def flushcommands(self):
"""Flush all queued commands. """Flush all queued commands.
Returns an iterable of frames that should be sent over the wire. Returns an iterable of frames that should be sent over the wire.
def _processframe(self, frame): def _processframe(self, frame):
"""Process a single read frame.""" """Process a single read frame."""
action, meta = self._reactor.onframerecv(frame) action, meta = self._reactor.onframerecv(frame)
if action == 'error': if action == 'error':
e = error.RepoError(meta['message']) e = error.RepoError(meta['message'])
if frame.requestid in self._responses:
self._responses[frame.requestid]._oninputcomplete()
if frame.requestid in self._futures: if frame.requestid in self._futures:
self._futures[frame.requestid].set_exception(e) self._futures[frame.requestid].set_exception(e)
del self._futures[frame.requestid]
else: else:
raise e raise e
return return
if frame.requestid not in self._requests: if frame.requestid not in self._requests:
raise error.ProgrammingError( raise error.ProgrammingError(
'received frame for unknown request; this is either a bug in ' 'received frame for unknown request; this is either a bug in '
'the clientreactor not screening for this or this instance was ' 'the clientreactor not screening for this or this instance was '
'never told about this request: %r' % frame) 'never told about this request: %r' % frame)
response = self._responses[frame.requestid] response = self._responses[frame.requestid]
if action == 'responsedata': if action == 'responsedata':
# Any failures processing this frame should bubble up to the # Any failures processing this frame should bubble up to the
# future tracking the request. # future tracking the request.
try: try:
self._processresponsedata(frame, meta, response) self._processresponsedata(frame, meta, response)
except BaseException as e: except BaseException as e:
self._futures[frame.requestid].set_exception(e) self._futures[frame.requestid].set_exception(e)
del self._futures[frame.requestid]
response._oninputcomplete()
else: else:
raise error.ProgrammingError( raise error.ProgrammingError(
'unhandled action from clientreactor: %s' % action) 'unhandled action from clientreactor: %s' % action)
def _processresponsedata(self, frame, meta, response): def _processresponsedata(self, frame, meta, response):
# This buffers all data until end of stream is received. This # This can raise. The caller can handle it.
# is bad for performance. response._onresponsedata(meta['data'])
# TODO make response data streamable
response.b.write(meta['data'])
if meta['eos']: if meta['eos']:
# If the command has a decoder, resolve the future to the response._oninputcomplete()
# decoded value. Otherwise resolve to the rich response object. del self._requests[frame.requestid]
decoder = COMMAND_DECODERS.get(response.command)
# TODO consider always resolving the overall status map.
if decoder:
objs = response.cborobjects()
overall = next(objs)
if overall['status'] == 'ok': # If the command has a decoder, we wait until all input has been
self._futures[frame.requestid].set_result(decoder(objs)) # received before resolving the future. Otherwise we resolve the
else: # future immediately.
atoms = [{'msg': overall['error']['message']}] if frame.requestid not in self._futures:
if 'args' in overall['error']: return
atoms[0]['args'] = overall['error']['args']
e = error.RepoError(formatrichmessage(atoms))
self._futures[frame.requestid].set_exception(e)
else:
self._futures[frame.requestid].set_result(response)
del self._requests[frame.requestid] if response.command not in COMMAND_DECODERS:
self._futures[frame.requestid].set_result(response.objects())
del self._futures[frame.requestid]
elif response._inputcomplete:
decoded = COMMAND_DECODERS[response.command](response.objects())
self._futures[frame.requestid].set_result(decoded)
del self._futures[frame.requestid] del self._futures[frame.requestid]
def decodebranchmap(objs): def decodebranchmap(objs):
# Response should be a single CBOR map of branch name to array of nodes. # Response should be a single CBOR map of branch name to array of nodes.
bm = next(objs) bm = next(objs)
return {encoding.tolocal(k): v for k, v in bm.items()} return {encoding.tolocal(k): v for k, v in bm.items()}

tests/test-http-api-httpv2.t

s> \r\n s> \r\n
received frame(size=31; request=1; stream=2; streamflags=; type=command-response; flags=continuation) received frame(size=31; request=1; stream=2; streamflags=; type=command-response; flags=continuation)
s> 8\r\n s> 8\r\n
s> \x00\x00\x00\x01\x00\x02\x002 s> \x00\x00\x00\x01\x00\x02\x002
s> \r\n s> \r\n
s> 0\r\n s> 0\r\n
s> \r\n s> \r\n
received frame(size=0; request=1; stream=2; streamflags=; type=command-response; flags=eos) received frame(size=0; request=1; stream=2; streamflags=; type=command-response; flags=eos)
response: [ response: gen[
{
b'status': b'ok'
},
b'customreadonly bytes response' b'customreadonly bytes response'
] ]
Request to read-write command fails because server is read-only by default Request to read-write command fails because server is read-only by default
GET to read-write request yields 405 GET to read-write request yields 405
$ sendhttpraw << EOF $ sendhttpraw << EOF

tests/test-wireproto-command-capabilities.t

s> \r\n s> \r\n
received frame(size=452; request=1; stream=2; streamflags=; type=command-response; flags=continuation) received frame(size=452; request=1; stream=2; streamflags=; type=command-response; flags=continuation)
s> 8\r\n s> 8\r\n
s> \x00\x00\x00\x01\x00\x02\x002 s> \x00\x00\x00\x01\x00\x02\x002
s> \r\n s> \r\n
s> 0\r\n s> 0\r\n
s> \r\n s> \r\n
received frame(size=0; request=1; stream=2; streamflags=; type=command-response; flags=eos) received frame(size=0; request=1; stream=2; streamflags=; type=command-response; flags=eos)
response: [ response: gen[
{
b'status': b'ok'
},
{ {
b'commands': { b'commands': {
b'branchmap': { b'branchmap': {
b'args': {}, b'args': {},
b'permissions': [ b'permissions': [
b'pull' b'pull'
] ]
}, },