( )⚙ D2064 wireprotoserver: document and improve the httplib workaround

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2064

wireprotoserver: document and improve the httplib workaround
ClosedPublic

Authored by indygreg on Feb 6 2018, 4:04 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG6010fe1da619: wireprotoserver: document and improve the httplib workaround
Summary

This workaround dates all the way back to a42d27bc809d in 2008.
The code is esoteric enough to warrant an inline explanation.
So I've added one.

At the time the code was written, the only wire protocol command
that accepted an HTTP request body was "unbundle." In the years
since, we've grown the ability to accept command arguments via
HTTP POST requests. So, the code has been changed to apply the
httplib workaround to all HTTP POST requests.

While staring at this code, I realized that the HTTP response
body in case of error is always the same. And, it appears to
mimic the behavior of a failed call to the "unbundle" command.
Since we can hit this code path on theoretically any protocol
request (since self.check_perm accepts custom auth checking
functions which may raise), I'm having a hard time believing
that clients react well to an "unbundle" response payload on
any wire protocol command. I wouldn't be surprised if our test
coverage for this feature only covers HTTP POST calls to
"unbundle." In other words, the experimental support for sending
arguments via HTTP POST request bodies may result in badness on
the client. Something to investigate another time perhaps...

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.Feb 6 2018, 4:04 PM
durin42 accepted this revision.Feb 7 2018, 5:19 PM
durin42 added a subscriber: durin42.

I'm so sorry for this technical debt, even if it's httplib's fault. :/

This revision is now accepted and ready to land.Feb 7 2018, 5:19 PM
Closed by commit rHG6010fe1da619: wireprotoserver: document and improve the httplib workaround (authored by indygreg). · Explain WhyFeb 7 2018, 5:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/wireprotoserver.py (21 lines)

Diff 5312

mercurial/wireprotoserver.py

req.respond(HTTP_OK, mediatype) req.respond(HTTP_OK, mediatype)
return gen return gen
elif isinstance(rsp, wireproto.pushres): elif isinstance(rsp, wireproto.pushres):
val = proto.restore() val = proto.restore()
rsp = '%d\n%s' % (rsp.res, val) rsp = '%d\n%s' % (rsp.res, val)
req.respond(HTTP_OK, HGTYPE, body=rsp) req.respond(HTTP_OK, HGTYPE, body=rsp)
return [] return []
elif isinstance(rsp, wireproto.pusherr): elif isinstance(rsp, wireproto.pusherr):
# drain the incoming bundle # This is the httplib workaround documented in _handlehttperror().
req.drain() req.drain()
proto.restore() proto.restore()
rsp = '0\n%s\n' % rsp.res rsp = '0\n%s\n' % rsp.res
req.respond(HTTP_OK, HGTYPE, body=rsp) req.respond(HTTP_OK, HGTYPE, body=rsp)
return [] return []
elif isinstance(rsp, wireproto.ooberror): elif isinstance(rsp, wireproto.ooberror):
rsp = rsp.message rsp = rsp.message
req.respond(HTTP_OK, HGERRTYPE, body=rsp) req.respond(HTTP_OK, HGERRTYPE, body=rsp)
return [] return []
raise error.ProgrammingError('hgweb.protocol internal failure', rsp) raise error.ProgrammingError('hgweb.protocol internal failure', rsp)
def _handlehttperror(e, req, cmd): def _handlehttperror(e, req, cmd):
"""Called when an ErrorResponse is raised during HTTP request processing.""" """Called when an ErrorResponse is raised during HTTP request processing."""
# A client that sends unbundle without 100-continue will
# break if we respond early. # Clients using Python's httplib are stateful: the HTTP client
if (cmd == 'unbundle' and # won't process an HTTP response until all request data is
# sent to the server. The intent of this code is to ensure
# we always read HTTP request data from the client, thus
# ensuring httplib transitions to a state that allows it to read
# the HTTP response. In other words, it helps prevent deadlocks
# on clients using httplib.
if (req.env[r'REQUEST_METHOD'] == r'POST' and
# But not if Expect: 100-continue is being used.
(req.env.get('HTTP_EXPECT', (req.env.get('HTTP_EXPECT',
'').lower() != '100-continue') or '').lower() != '100-continue') or
# Or the non-httplib HTTP library is being advertised by
# the client.
req.env.get('X-HgHttp2', '')): req.env.get('X-HgHttp2', '')):
req.drain() req.drain()
else: else:
req.headers.append((r'Connection', r'Close')) req.headers.append((r'Connection', r'Close'))
# TODO This response body assumes the failed command was
# "unbundle." That assumption is not always valid.
req.respond(e, HGTYPE, body='0\n%s\n' % e) req.respond(e, HGTYPE, body='0\n%s\n' % e)
return '' return ''
class sshserver(abstractserverproto): class sshserver(abstractserverproto):
def __init__(self, ui, repo): def __init__(self, ui, repo):
self._ui = ui self._ui = ui
self._repo = repo self._repo = repo