Patchwork get-with-headers: don't block indefinitely if the server had an internal error

login
register
mail settings
Submitter Javi Merino
Date Oct. 4, 2013, 8:12 a.m.
Message ID <20131004081228.GA2840@tesla>
Download mbox | patch
Permalink /patch/2730/
State Accepted
Commit ba6577a196560d4a2925fc844520d75dedb28861
Headers show

Comments

Javi Merino - Oct. 4, 2013, 8:12 a.m.
On Thu, Oct 03, 2013 at 10:45:51AM -0400, Augie Fackler wrote:
> On Wed, Oct 02, 2013 at 11:03:19PM +0100, Javi Merino wrote:
> > # HG changeset patch
> > # User Javi Merino <cibervicho@gmail.com>
> > # Date 1380750392 -3600
> > #      Wed Oct 02 22:46:32 2013 +0100
> > # Node ID c135513fb69b57d869eb98c23cef2f5c3fab54e7
> > # Parent  1935e8383a9e1bd1ac6809ad1ecafd42dd7d58b2
> > get-with-headers: don't block indefinitely if the server had an internal error
> >
> > If the server had an internal error and returned 500, there's nothing
> > to read, so "response.read()" blocks indefinitely.  Only output the
> > response if there's really a response.
> 
> Bwuh? There can be a body on a 500. This strikes me as probably
> wrong. Can you give an example of a case that hangs?

If mercurial fails to install templates, some tests in the testsuite
hang.  You can force the not installing templates when running the
testsuite with:

------8<
------8<

$ make test-largefiles.t TESTFLAGS=-d
cd tests && python run-tests.py -d test-largefiles.t
python hash seed: 1431828856
+ echo SALT1380873708.27 0 0
SALT1380873708.27 0 0
+ USERCACHE=/tmp/hgtests.d54IMW/child0/test-largefiles.t/cache
+ export USERCACHE
+ echo SALT1380873708.27 1 0
SALT1380873708.27 1 0
[...]
+ hg serve -d -p 20059 --pid-file ../hg.pid
+ echo SALT1380873708.27 223 0
SALT1380873708.27 223 0
+ cat ../hg.pid
+ echo SALT1380873708.27 224 0
SALT1380873708.27 224 0
+ /root/mercurial-2.7.2/tests/get-with-headers.py 127.0.0.1:20059 file/tip/?style=raw
500 Internal Server Error

and "response.read()" in get-with-headers.py:39 blocks waiting for
data that will never come.

Admittedly this is harder to reproduce than what I originally thought,
cheers,
Javi
Augie Fackler - Oct. 5, 2013, 9:22 p.m.
On Oct 4, 2013, at 4:12 AM, Javi Merino <cibervicho@gmail.com> wrote:

> On Thu, Oct 03, 2013 at 10:45:51AM -0400, Augie Fackler wrote:
>> On Wed, Oct 02, 2013 at 11:03:19PM +0100, Javi Merino wrote:
>>> # HG changeset patch
>>> # User Javi Merino <cibervicho@gmail.com>
>>> # Date 1380750392 -3600
>>> #      Wed Oct 02 22:46:32 2013 +0100
>>> # Node ID c135513fb69b57d869eb98c23cef2f5c3fab54e7
>>> # Parent  1935e8383a9e1bd1ac6809ad1ecafd42dd7d58b2
>>> get-with-headers: don't block indefinitely if the server had an internal error
>>> 
>>> If the server had an internal error and returned 500, there's nothing
>>> to read, so "response.read()" blocks indefinitely.  Only output the
>>> response if there's really a response.
>> 
>> Bwuh? There can be a body on a 500. This strikes me as probably
>> wrong. Can you give an example of a case that hangs?
> 
> If mercurial fails to install templates, some tests in the testsuite
> hang.  You can force the not installing templates when running the
> testsuite with:

This is the wrong fix - Mercurial shouldn't produce invalid 500 responses if the templates are missing.

Patch

--- a/setup.py
+++ b/setup.py
@@ -478,14 +478,6 @@  packagedata = {'mercurial': ['locale/*/L
 def ordinarypath(p):
     return p and p[0] != '.' and p[-1] != '~'
 
-for root in ('templates',):
-    for curdir, dirs, files in os.walk(os.path.join('mercurial', root)):
-        curdir = curdir.split(os.sep, 1)[1]
-        dirs[:] = filter(ordinarypath, dirs)
-        for f in filter(ordinarypath, files):
-            f = os.path.join(curdir, f)
-            packagedata['mercurial'].append(f)
-
 datafiles = []
 setupversion = version
 extra = {}