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

login
register
mail settings
Submitter phabricator
Date April 24, 2018, 9:16 p.m.
Message ID <differential-rev-PHID-DREV-vqe32gdxn4i2exioodfu-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31222/
State Superseded
Headers show

Comments

phabricator - April 24, 2018, 9:16 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  An unintended side-effect of https://phab.mercurial-scm.org/rHGf0a851542a0596b6746bf44a5aa12ecd189a0a25 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3427

AFFECTED FILES
  mercurial/hgweb/hgwebdir_mod.py
  mercurial/hgweb/request.py
  tests/test-push-http.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - April 25, 2018, 1:13 a.m.
mharbison72 added a comment.


  > 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.

INLINE COMMENTS

> test-push-http.t:399
> +  > [extensions]
> +  > showstack = $TESTDIR/../contrib/showstack.py
> +  > EOF

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3427

To: indygreg, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - April 25, 2018, 4:08 a.m.
indygreg added inline comments.

INLINE COMMENTS

> mharbison72 wrote in 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?

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3427

To: indygreg, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - April 25, 2018, 1:47 p.m.
durin42 added a comment.


  queued for stable with showstack dropped

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3427

To: indygreg, #hg-reviewers
Cc: durin42, mharbison72, mercurial-devel

Patch

diff --git a/tests/test-push-http.t b/tests/test-push-http.t
--- a/tests/test-push-http.t
+++ b/tests/test-push-http.t
@@ -380,3 +380,49 @@ 
 #endif
 
   $ 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
+  > 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 ..
diff --git a/mercurial/hgweb/request.py b/mercurial/hgweb/request.py
--- a/mercurial/hgweb/request.py
+++ b/mercurial/hgweb/request.py
@@ -124,7 +124,7 @@ 
     # WSGI environment dict, unmodified.
     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.
 
     WSGI defines request attributes via environment variables. This function
@@ -144,6 +144,9 @@ 
     if the request were to ``http://myserver:9000/prefix/rev/@``. In other
     words, ``wsgi.url_scheme``, ``SERVER_NAME``, ``SERVER_PORT``, and
     ``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.
 
@@ -307,9 +310,10 @@ 
     if 'CONTENT_TYPE' in env and 'HTTP_CONTENT_TYPE' not in env:
         headers['Content-Type'] = env['CONTENT_TYPE']
 
-    bodyfh = env['wsgi.input']
-    if 'Content-Length' in headers:
-        bodyfh = util.cappedreader(bodyfh, int(headers['Content-Length']))
+    if bodyfh is None:
+        bodyfh = env['wsgi.input']
+        if 'Content-Length' in headers:
+            bodyfh = util.cappedreader(bodyfh, int(headers['Content-Length']))
 
     return parsedrequest(method=env['REQUEST_METHOD'],
                          url=fullurl, baseurl=baseurl,
diff --git a/mercurial/hgweb/hgwebdir_mod.py b/mercurial/hgweb/hgwebdir_mod.py
--- a/mercurial/hgweb/hgwebdir_mod.py
+++ b/mercurial/hgweb/hgwebdir_mod.py
@@ -428,7 +428,10 @@ 
                                 uenv.iteritems()}
                     req = requestmod.parserequestfromenv(
                         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:
                         # ensure caller gets private copy of ui
                         repo = hg.repository(self.ui.copy(), real)