( )⚙ D231 httppeer: add support for httppostargs when we're sending a file

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D231

httppeer: add support for httppostargs when we're sending a file
ClosedPublic

Authored by durin42 on Aug 4 2017, 5:05 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG3c91cc0c5fde: httppeer: add support for httppostargs when we're sending a file
Summary

This is probably only used in the 'unbundle' command, but the code
ended up being cleaner to make it generic and treat *all* httppostargs
with a non-args request body as though they were file-like in
nature. It also means we get test coverage more or less for free. A
previous version of this change didn't use io.BytesIO, and it was a
lot more complicated.

This also fixes a server-side bug, so anyone using httppostargs should
update all of their servers to this revision or later *before* this
gets to their clients, otherwise servers will hang trying to over-read
the POST body.

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

durin42 created this revision.Aug 4 2017, 5:05 PM
martinvonz added a subscriber: martinvonz.Aug 5 2017, 1:04 AM
martinvonz added inline comments.
mercurial/httppeer.py
95

I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so please update)

96

It doesn't matter much since it's just a programming error if it happens, but how will these arguments to ValueError be rendered?

105

i read that negative amount means the same thing (read all). do we care to follow that contract here?

111

nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time _multifile.read() (and it does look like that will be the normal case, that the reader will read exactly the size of the first "file" first)

125

also need to set self._index=0, no?

213–214

Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok and args" instead (because bool(strargs) == bool(args) AFAICT)

durin42 marked 3 inline comments as done.Aug 8 2017, 10:41 AM
durin42 updated this revision to Diff 627.
durin42 added inline comments.Aug 8 2017, 10:41 AM
mercurial/httppeer.py
96

ValueError: ('_multifile only supports file objects that have a length but this one does not:', <type 'file'>, <open file 'README', mode 'r' at 0x110ff0b70>)

It ain't pretty, but it's got all the information we need.

111

No, because consider these file buffers:

'foo', 'bar'

if we read(2), we'll pull 2 bytes off the first buffer, which is not yet exhausted, but got == amt.

durin42 updated this revision to Diff 745.Aug 10 2017, 4:04 PM
martinvonz accepted this revision.Aug 15 2017, 4:31 PM
This revision is now accepted and ready to land.Aug 15 2017, 4:31 PM
Closed by commit rHG3c91cc0c5fde: httppeer: add support for httppostargs when we're sending a file (authored by durin42). · Explain WhyAug 15 2017, 4:55 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Aug 21 2017, 9:21 AM
yuja added inline comments.
mercurial/httppeer.py
105

Just nits:

  • read(0) should return an empty string.
  • None <= 0 is TypeError on Python 3.
223

Does this mean data must have length attribute if it isn't
a string? A plain file object has no such attribute.

Revision Contents
Changeset List

PathPackages
Mmercurial/hgweb/protocol.py (3 lines)
Mmercurial/httppeer.py (62 lines)

Diff 949

mercurial/hgweb/protocol.py

self.req.read(postlen), keep_blank_values=True)) self.req.read(postlen), keep_blank_values=True))
return args return args
argvalue = decodevaluefromheaders(self.req, 'X-HgArg') argvalue = decodevaluefromheaders(self.req, 'X-HgArg')
args.update(cgi.parse_qs(argvalue, keep_blank_values=True)) args.update(cgi.parse_qs(argvalue, keep_blank_values=True))
return args return args
def getfile(self, fp): def getfile(self, fp):
length = int(self.req.env['CONTENT_LENGTH']) length = int(self.req.env['CONTENT_LENGTH'])
# If httppostargs is used, we need to read Content-Length
# minus the amount that was consumed by args.
length -= int(self.req.env.get('HTTP_X_HGARGS_POST', 0))
for s in util.filechunkiter(self.req, limit=length): for s in util.filechunkiter(self.req, limit=length):
fp.write(s) fp.write(s)
def redirect(self): def redirect(self):
self.oldio = self.ui.fout, self.ui.ferr self.oldio = self.ui.fout, self.ui.ferr
self.ui.ferr = self.ui.fout = stringio() self.ui.ferr = self.ui.fout = stringio()
def restore(self): def restore(self):
val = self.ui.fout.getvalue() val = self.ui.fout.getvalue()
self.ui.ferr, self.ui.fout = self.oldio self.ui.ferr, self.ui.fout = self.oldio

mercurial/httppeer.py

# httppeer.py - HTTP repository proxy classes for mercurial # httppeer.py - HTTP repository proxy classes for mercurial
# #
# Copyright 2005, 2006 Matt Mackall <mpm@selenic.com> # Copyright 2005, 2006 Matt Mackall <mpm@selenic.com>
# Copyright 2006 Vadim Gelfer <vadim.gelfer@gmail.com> # Copyright 2006 Vadim Gelfer <vadim.gelfer@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 errno import errno
import io
import os import os
import socket import socket
import struct import struct
import tempfile import tempfile
from .i18n import _ from .i18n import _
from .node import nullid from .node import nullid
from . import ( from . import (
raise error.PeerTransportError( raise error.PeerTransportError(
_('HTTP request error (%s)') % e, _('HTTP request error (%s)') % e,
hint=_('this may be an intermittent network failure; ' hint=_('this may be an intermittent network failure; '
'if the error persists, consider contacting the ' 'if the error persists, consider contacting the '
'network or server operator')) 'network or server operator'))
resp.__class__ = readerproxy resp.__class__ = readerproxy
class _multifile(object):
def __init__(self, *fileobjs):
for f in fileobjs:
if not util.safehasattr(f, 'length'):
raise ValueError(
'_multifile only supports file objects that '
martinvonzUnsubmitted
Done

I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so please update)

martinvonz: I'm guessing 'fileprepender' here and a few placed below is the old name for '_multifile' (so…
'have a length but this one does not:', type(f), f)
martinvonzUnsubmitted
Not Done

It doesn't matter much since it's just a programming error if it happens, but how will these arguments to ValueError be rendered?

martinvonz: It doesn't matter much since it's just a programming error if it happens, but how will these…
durin42AuthorUnsubmitted
Not Done

ValueError: ('_multifile only supports file objects that have a length but this one does not:', <type 'file'>, <open file 'README', mode 'r' at 0x110ff0b70>)

It ain't pretty, but it's got all the information we need.

durin42: ValueError: ('_multifile only supports file objects that have a length but this one does not:'…
self._fileobjs = fileobjs
self._index = 0
@property
def length(self):
return sum(f.length for f in self._fileobjs)
def read(self, amt=None):
if amt <= 0:
martinvonzUnsubmitted
Not Done

i read that negative amount means the same thing (read all). do we care to follow that contract here?

martinvonz: i read that negative amount means the same thing (read all). do we care to follow that contract…
yujaUnsubmitted
Not Done

Just nits:

  • read(0) should return an empty string.
  • None <= 0 is TypeError on Python 3.
yuja: Just nits: * `read(0)` should return an empty string. * `None <= 0` is TypeError on Python 3.
return ''.join(f.read() for f in self._fileobjs)
parts = []
while amt and self._index < len(self._fileobjs):
parts.append(self._fileobjs[self._index].read(amt))
got = len(parts[-1])
if got < amt:
martinvonzUnsubmitted
Not Done

nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time _multifile.read() (and it does look like that will be the normal case, that the reader will read exactly the size of the first "file" first)

martinvonz: nit: i think this can be "if got <= amt" to avoid an unnecessary 0-length read the next time…
durin42AuthorUnsubmitted
Not Done

No, because consider these file buffers:

'foo', 'bar'

if we read(2), we'll pull 2 bytes off the first buffer, which is not yet exhausted, but got == amt.

durin42: No, because consider these file buffers: 'foo', 'bar' if we read(2), we'll pull 2 bytes off…
self._index += 1
amt -= got
return ''.join(parts)
def seek(self, offset, whence=os.SEEK_SET):
if whence != os.SEEK_SET:
raise NotImplementedError(
'_multifile does not support anything other'
' than os.SEEK_SET for whence on seek()')
if offset != 0:
raise NotImplementedError(
'_multifile only supports seeking to start, but that '
'could be fixed if you need it')
for f in self._fileobjs:
martinvonzUnsubmitted
Done

also need to set self._index=0, no?

martinvonz: also need to set self._index=0, no?
f.seek(0)
self._index = 0
class httppeer(wireproto.wirepeer): class httppeer(wireproto.wirepeer):
def __init__(self, ui, path): def __init__(self, ui, path):
self._path = path self._path = path
self._caps = None self._caps = None
self._urlopener = None self._urlopener = None
self._requestbuilder = None self._requestbuilder = None
u = util.url(path) u = util.url(path)
if u.query or u.fragment: if u.query or u.fragment:
self.ui.debug("sending %s command\n" % cmd) self.ui.debug("sending %s command\n" % cmd)
q = [('cmd', cmd)] q = [('cmd', cmd)]
headersize = 0 headersize = 0
varyheaders = [] varyheaders = []
# Important: don't use self.capable() here or else you end up # Important: don't use self.capable() here or else you end up
# with infinite recursion when trying to look up capabilities # with infinite recursion when trying to look up capabilities
# for the first time. # for the first time.
postargsok = self._caps is not None and 'httppostargs' in self._caps postargsok = self._caps is not None and 'httppostargs' in self._caps
# TODO: support for httppostargs when data is a file-like if postargsok and args:
# object rather than a basestring
canmungedata = not data or isinstance(data, basestring)
if postargsok and canmungedata:
strargs = urlreq.urlencode(sorted(args.items())) strargs = urlreq.urlencode(sorted(args.items()))
if strargs:
if not data: if not data:
martinvonzUnsubmitted
Done

Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok and args" instead (because bool(strargs) == bool(args) AFAICT)

martinvonz: Nit: to reduce indentation, it looks like you can change the condition above to "if postargsok…
data = strargs data = strargs
elif isinstance(data, basestring): else:
data = strargs + data if isinstance(data, basestring):
i = io.BytesIO(data)
i.length = len(data)
data = i
argsio = io.BytesIO(strargs)
argsio.length = len(strargs)
data = _multifile(argsio, data)
yujaUnsubmitted
Not Done

Does this mean data must have length attribute if it isn't
a string? A plain file object has no such attribute.

yuja: Does this mean `data` must have `length` attribute if it isn't a string? A plain file object…
headers['X-HgArgs-Post'] = len(strargs) headers['X-HgArgs-Post'] = len(strargs)
else: else:
if len(args) > 0: if len(args) > 0:
httpheader = self.capable('httpheader') httpheader = self.capable('httpheader')
if httpheader: if httpheader:
headersize = int(httpheader.split(',', 1)[0]) headersize = int(httpheader.split(',', 1)[0])
if headersize > 0: if headersize > 0:
# The headers can typically carry more data than the URL. # The headers can typically carry more data than the URL.
encargs = urlreq.urlencode(sorted(args.items())) encargs = urlreq.urlencode(sorted(args.items()))