This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3427

hgweb: reuse body file object when hgwebdir calls hgweb (issue5851)
ClosedPublic

Authored by indygreg on Apr 24 2018, 5:16 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG877185de62cf: hgweb: reuse body file object when hgwebdir calls hgweb (issue5851)
Summary

An unintended side-effect of f0a851542a05 was that the request body
file object (which uses a util.cappedreader) was constructed twice
when hgwebdir called into hgweb. Since we attempt to read all remaining
data from this file object when Content-Length is defined and since there
were two instances of this object and the client supplied no additional
data to read, this resulted in deadlock.

The fix implemented in this commit is to reuse the request body file
object when it is passed from hgwebdir to hgweb.

A test demonstrating hg clone and hg push via hgwebdir has been
added. Without this patch, the test hangs when doing hg clone.
Surprisingly, this must mean that we have effectively no test coverage
of the wire protocol when run via hgwebdir.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Apr 24 2018, 5:16 PM
mharbison72 added a subscriber: mharbison72.Apr 24 2018, 9:13 PM

A test demonstrating hg clone and hg push via hgwebdir has been
added. Without this patch, the test hangs when doing hg clone.
Surprisingly, this must mean that we have effectively no test coverage
of the wire protocol when run via hgwebdir.

I thought that hgwebdir testing is pretty light too, but there should be at least some additional coverage with subrepos. But maybe something is subtly different. For example, there's not an obvious difference between this new clone test, and this existing one:

https://www.mercurial-scm.org/repo/hg/file/8c8b6d13949a/tests/test-subrepo-recursion.t#l266

While there's no prefix on this clone, there is on in its subrepo.

tests/test-push-http.t
399

This needs to be excluded on Windows. Maybe I should just trap the error that gets raised, so that this loads but does nothing?

indygreg added inline comments.Apr 25 2018, 12:08 AM
tests/test-push-http.t
399

Oops. This was left over from my debugging. It can safely be removed.

durin42 added a subscriber: durin42.Apr 25 2018, 9:47 AM

queued for stable with showstack dropped

Closed by commit rHG877185de62cf: hgweb: reuse body file object when hgwebdir calls hgweb (issue5851) (authored by indygreg). · Explain WhyApr 25 2018, 9:48 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 8426

mercurial/hgweb/hgwebdir_mod.py

# Re-parse the WSGI environment to take into account our # Re-parse the WSGI environment to take into account our
# repository path component. # repository path component.
uenv = req.rawenv uenv = req.rawenv
if pycompat.ispy3: if pycompat.ispy3:
uenv = {k.decode('latin1'): v for k, v in uenv = {k.decode('latin1'): v for k, v in
uenv.iteritems()} uenv.iteritems()}
req = requestmod.parserequestfromenv( req = requestmod.parserequestfromenv(
uenv, reponame=virtualrepo, uenv, reponame=virtualrepo,
altbaseurl=self.ui.config('web', 'baseurl')) altbaseurl=self.ui.config('web', 'baseurl'),
# Reuse wrapped body file object otherwise state
# tracking can get confused.
bodyfh=req.bodyfh)
try: try:
# ensure caller gets private copy of ui # ensure caller gets private copy of ui
repo = hg.repository(self.ui.copy(), real) repo = hg.repository(self.ui.copy(), real)
return hgweb_mod.hgweb(repo).run_wsgi(req, res) return hgweb_mod.hgweb(repo).run_wsgi(req, res)
except IOError as inst: except IOError as inst:
msg = encoding.strtolocal(inst.strerror) msg = encoding.strtolocal(inst.strerror)
raise ErrorResponse(HTTP_SERVER_ERROR, msg) raise ErrorResponse(HTTP_SERVER_ERROR, msg)
except error.RepoError as inst: except error.RepoError as inst:

mercurial/hgweb/request.py

# wsgiref.headers.Headers instance. Operates like a dict with case # wsgiref.headers.Headers instance. Operates like a dict with case
# insensitive keys. # insensitive keys.
headers = attr.ib() headers = attr.ib()
# Request body input stream. # Request body input stream.
bodyfh = attr.ib() bodyfh = attr.ib()
# WSGI environment dict, unmodified. # WSGI environment dict, unmodified.
rawenv = attr.ib() rawenv = attr.ib()
def parserequestfromenv(env, reponame=None, altbaseurl=None): def parserequestfromenv(env, reponame=None, altbaseurl=None, bodyfh=None):
"""Parse URL components from environment variables. """Parse URL components from environment variables.
WSGI defines request attributes via environment variables. This function WSGI defines request attributes via environment variables. This function
parses the environment variables into a data structure. parses the environment variables into a data structure.
If ``reponame`` is defined, the leading path components matching that If ``reponame`` is defined, the leading path components matching that
string are effectively shifted from ``PATH_INFO`` to ``SCRIPT_NAME``. string are effectively shifted from ``PATH_INFO`` to ``SCRIPT_NAME``.
This simulates the world view of a WSGI application that processes This simulates the world view of a WSGI application that processes
requests from the base URL of a repo. requests from the base URL of a repo.
If ``altbaseurl`` (typically comes from ``web.baseurl`` config option) If ``altbaseurl`` (typically comes from ``web.baseurl`` config option)
is defined, it is used - instead of the WSGI environment variables - for is defined, it is used - instead of the WSGI environment variables - for
constructing URL components up to and including the WSGI application path. constructing URL components up to and including the WSGI application path.
For example, if the current WSGI application is at ``/repo`` and a request For example, if the current WSGI application is at ``/repo`` and a request
is made to ``/rev/@`` with this argument set to is made to ``/rev/@`` with this argument set to
``http://myserver:9000/prefix``, the URL and path components will resolve as ``http://myserver:9000/prefix``, the URL and path components will resolve as
if the request were to ``http://myserver:9000/prefix/rev/@``. In other if the request were to ``http://myserver:9000/prefix/rev/@``. In other
words, ``wsgi.url_scheme``, ``SERVER_NAME``, ``SERVER_PORT``, and words, ``wsgi.url_scheme``, ``SERVER_NAME``, ``SERVER_PORT``, and
``SCRIPT_NAME`` are all effectively replaced by components from this URL. ``SCRIPT_NAME`` are all effectively replaced by components from this URL.
``bodyfh`` can be used to specify a file object to read the request body
from. If not defined, ``wsgi.input`` from the environment dict is used.
""" """
# PEP 3333 defines the WSGI spec and is a useful reference for this code. # PEP 3333 defines the WSGI spec and is a useful reference for this code.
# We first validate that the incoming object conforms with the WSGI spec. # We first validate that the incoming object conforms with the WSGI spec.
# We only want to be dealing with spec-conforming WSGI implementations. # We only want to be dealing with spec-conforming WSGI implementations.
# TODO enable this once we fix internal violations. # TODO enable this once we fix internal violations.
#wsgiref.validate.check_environ(env) #wsgiref.validate.check_environ(env)
# this, since a consumer will either either value to determine how many # this, since a consumer will either either value to determine how many
# bytes are available to read. # bytes are available to read.
if 'CONTENT_LENGTH' in env and 'HTTP_CONTENT_LENGTH' not in env: if 'CONTENT_LENGTH' in env and 'HTTP_CONTENT_LENGTH' not in env:
headers['Content-Length'] = env['CONTENT_LENGTH'] headers['Content-Length'] = env['CONTENT_LENGTH']
if 'CONTENT_TYPE' in env and 'HTTP_CONTENT_TYPE' not in env: if 'CONTENT_TYPE' in env and 'HTTP_CONTENT_TYPE' not in env:
headers['Content-Type'] = env['CONTENT_TYPE'] headers['Content-Type'] = env['CONTENT_TYPE']
if bodyfh is None:
bodyfh = env['wsgi.input'] bodyfh = env['wsgi.input']
if 'Content-Length' in headers: if 'Content-Length' in headers:
bodyfh = util.cappedreader(bodyfh, int(headers['Content-Length'])) bodyfh = util.cappedreader(bodyfh, int(headers['Content-Length']))
return parsedrequest(method=env['REQUEST_METHOD'], return parsedrequest(method=env['REQUEST_METHOD'],
url=fullurl, baseurl=baseurl, url=fullurl, baseurl=baseurl,
advertisedurl=advertisedfullurl, advertisedurl=advertisedfullurl,
advertisedbaseurl=advertisedbaseurl, advertisedbaseurl=advertisedbaseurl,
urlscheme=env['wsgi.url_scheme'], urlscheme=env['wsgi.url_scheme'],
remoteuser=env.get('REMOTE_USER'), remoteuser=env.get('REMOTE_USER'),
remotehost=env.get('REMOTE_HOST'), remotehost=env.get('REMOTE_HOST'),

tests/test-push-http.t

remote: adding manifests remote: adding manifests
remote: adding file changes remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files remote: added 1 changesets with 1 changes to 1 files
% serve errors % serve errors
#endif #endif
$ cd .. $ cd ..
Pushing via hgwebdir works
$ hg init hgwebdir
$ cd hgwebdir
$ echo 0 > a
$ hg -q commit -A -m initial
$ cd ..
$ cat > web.conf << EOF
> [paths]
> / = *
> [web]
> push_ssl = false
> allow_push = *
> [extensions]
> showstack = $TESTDIR/../contrib/showstack.py
mharbison72Unsubmitted
Not Done

This needs to be excluded on Windows. Maybe I should just trap the error that gets raised, so that this loads but does nothing?

mharbison72: This needs to be excluded on Windows. Maybe I should just trap the error that gets raised, so…
indygregAuthorUnsubmitted
Not Done

Oops. This was left over from my debugging. It can safely be removed.

indygreg: Oops. This was left over from my debugging. It can safely be removed.
> EOF
$ hg serve --web-conf web.conf -p $HGPORT -d --pid-file hg.pid
$ cat hg.pid > $DAEMON_PIDS
$ hg clone http://localhost:$HGPORT/hgwebdir hgwebdir-local
requesting all changes
adding changesets
adding manifests
adding file changes
added 1 changesets with 1 changes to 1 files
new changesets 98a3f8f02ba7
updating to branch default
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ cd hgwebdir-local
$ echo commit > a
$ hg commit -m 'local commit'
$ hg push
pushing to http://localhost:$HGPORT/hgwebdir
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
$ killdaemons.py
$ cd ..