Patchwork [evolve-ext] evolve: prevent a crash in httpclient_pushobsmarkers() when pushing

login
register
mail settings
Submitter Matt Harbison
Date March 6, 2015, 1:43 a.m.
Message ID <0828a7e7f402374dca9c.1425606212@Envy>
Download mbox | patch
Permalink /patch/7906/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Matt Harbison - March 6, 2015, 1:43 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1425603727 18000
#      Thu Mar 05 20:02:07 2015 -0500
# Branch stable
# Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
# Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
evolve: prevent a crash in httpclient_pushobsmarkers() when pushing

I've been running into a crash when pushing from my hg repo in a Fedora 16 VM to
Win7 running 'hg serve', even with extensions disabled on both sides:

  ../hg push -r . pc
  pushing to http://192.168.1.4:8000/
  searching for changes
  no changes found
  pushing 2 obsolescence markers (263 bytes)
  ** unknown exception encountered, please report by visiting
  ...
  File "hg-evolve/hgext/evolve.py", line 2482, in _pushobsolete
    remote.evoext_pushobsmarkers_0(obsdata)
  File "hg-evolve/hgext/evolve.py", line 2522, in httpclient_pushobsmarkers
    ret, output = self._call('evoext_pushobsmarkers_0', data=obsfile)
  ValueError: too many values to unpack

I'm not sure how this repo differs from the one in the test suite, so I'm not
sure how to craft a test for this.  The failure occurs even when there _are_
csets to push.  There was no crash if no obsolete markers needed to be pushed.

At any rate, this code was stolen from httppeer._callpush(), where it calls
self._call().  The socket exception handling wasn't necessary to fix the crash,
but the calling code might as well be duplicated in its entirety.

A successful push with this patch looks like this.  Note the final line is _not_
in the output of the http push in test-simple4server.t:

  ../hg push -r . pc
  pushing to http://192.168.1.4:8000/
  searching for changes
  remote has heads on branch 'default' that are not known locally: 3af110194a0c
      56000e3ae44d 57ac6e51d290 7da4355c21b8 and 8 others
  remote: adding changesets
  remote: adding manifests
  remote: adding file changes
  remote: added 1 changesets with 0 changes to 1 files (+1 heads)
  pushing 4 obsolescence markers (525 bytes)
  remote: 2 obsolescence markers added
Pierre-Yves David - March 10, 2015, 2:50 a.m.
On 03/05/2015 05:43 PM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1425603727 18000
> #      Thu Mar 05 20:02:07 2015 -0500
> # Branch stable
> # Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
> # Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
> evolve: prevent a crash in httpclient_pushobsmarkers() when pushing

Pushing to main with an eyes brown raised.
Matt Harbison - March 10, 2015, 2:59 a.m.
On Mon, 09 Mar 2015 22:50:53 -0400, Pierre-Yves David  
<pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 03/05/2015 05:43 PM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1425603727 18000
>> #      Thu Mar 05 20:02:07 2015 -0500
>> # Branch stable
>> # Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
>> # Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
>> evolve: prevent a crash in httpclient_pushobsmarkers() when pushing
>
> Pushing to main with an eyes brown raised.

Given the previous code, I can't believe it used to work at all over http,  
even though the test says it does.  Any thoughts?  Any debug steps you  
think would be useful on this repo to see why it is different?

--Matt
Pierre-Yves David - March 10, 2015, 3:13 a.m.
On 03/09/2015 07:59 PM, Matt Harbison wrote:
> On Mon, 09 Mar 2015 22:50:53 -0400, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 03/05/2015 05:43 PM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1425603727 18000
>>> #      Thu Mar 05 20:02:07 2015 -0500
>>> # Branch stable
>>> # Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
>>> # Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
>>> evolve: prevent a crash in httpclient_pushobsmarkers() when pushing
>>
>> Pushing to main with an eyes brown raised.
>
> Given the previous code, I can't believe it used to work at all over
> http, even though the test says it does.  Any thoughts?  Any debug steps
> you think would be useful on this repo to see why it is different?


The tests are supposed to ensure that. If the http push is actually 
untested, making it tested sounds like a first step.
Matt Harbison - March 10, 2015, 3:48 a.m.
On Mon, 09 Mar 2015 23:13:39 -0400, Pierre-Yves David  
<pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 03/09/2015 07:59 PM, Matt Harbison wrote:
>> On Mon, 09 Mar 2015 22:50:53 -0400, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 03/05/2015 05:43 PM, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1425603727 18000
>>>> #      Thu Mar 05 20:02:07 2015 -0500
>>>> # Branch stable
>>>> # Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
>>>> # Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
>>>> evolve: prevent a crash in httpclient_pushobsmarkers() when pushing
>>>
>>> Pushing to main with an eyes brown raised.
>>
>> Given the previous code, I can't believe it used to work at all over
>> http, even though the test says it does.  Any thoughts?  Any debug steps
>> you think would be useful on this repo to see why it is different?
>
>
> The tests are supposed to ensure that. If the http push is actually  
> untested, making it tested sounds like a first step.

I think test-simple4server.t, line 90 covers it:

   $ hg push
   pushing to http://localhost:$HGPORT/
   searching for changes
   remote: adding changesets
   remote: adding manifests
   remote: adding file changes
   remote: added 1 changesets with 1 changes to 1 files (+1 heads)
   pushing 2 obsolescence markers (* bytes) (glob)

I can't believe specifying a path or revision like I did in the commit  
message demo makes any difference.  The code looked like it would fail any  
time there was a "pushing xx obsolete markers" line, so I must be missing  
something.

--Matt
Pierre-Yves David - March 10, 2015, 4:10 a.m.
On 03/09/2015 08:48 PM, Matt Harbison wrote:
> On Mon, 09 Mar 2015 23:13:39 -0400, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 03/09/2015 07:59 PM, Matt Harbison wrote:
>>> On Mon, 09 Mar 2015 22:50:53 -0400, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>>
>>>>
>>>> On 03/05/2015 05:43 PM, Matt Harbison wrote:
>>>>> # HG changeset patch
>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>> # Date 1425603727 18000
>>>>> #      Thu Mar 05 20:02:07 2015 -0500
>>>>> # Branch stable
>>>>> # Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
>>>>> # Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
>>>>> evolve: prevent a crash in httpclient_pushobsmarkers() when pushing
>>>>
>>>> Pushing to main with an eyes brown raised.
>>>
>>> Given the previous code, I can't believe it used to work at all over
>>> http, even though the test says it does.  Any thoughts?  Any debug steps
>>> you think would be useful on this repo to see why it is different?
>>
>>
>> The tests are supposed to ensure that. If the http push is actually
>> untested, making it tested sounds like a first step.
>
> I think test-simple4server.t, line 90 covers it:
>
>    $ hg push
>    pushing to http://localhost:$HGPORT/
>    searching for changes
>    remote: adding changesets
>    remote: adding manifests
>    remote: adding file changes
>    remote: added 1 changesets with 1 changes to 1 files (+1 heads)
>    pushing 2 obsolescence markers (* bytes) (glob)
>
> I can't believe specifying a path or revision like I did in the commit
> message demo makes any difference.  The code looked like it would fail
> any time there was a "pushing xx obsolete markers" line, so I must be
> missing something.

Your initial message is talking about situation when no changeset are to 
be pushed. This might be related ?
Matt Harbison - March 11, 2015, 12:27 a.m.
On Tue, 10 Mar 2015 00:10:11 -0400, Pierre-Yves David  
<pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 03/09/2015 08:48 PM, Matt Harbison wrote:
>> On Mon, 09 Mar 2015 23:13:39 -0400, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 03/09/2015 07:59 PM, Matt Harbison wrote:
>>>> On Mon, 09 Mar 2015 22:50:53 -0400, Pierre-Yves David
>>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On 03/05/2015 05:43 PM, Matt Harbison wrote:
>>>>>> # HG changeset patch
>>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>>> # Date 1425603727 18000
>>>>>> #      Thu Mar 05 20:02:07 2015 -0500
>>>>>> # Branch stable
>>>>>> # Node ID 0828a7e7f402374dca9cc089a9ff958fb478163c
>>>>>> # Parent  1e7c8046a9f461bda3c8f6003dae65e235af8bb6
>>>>>> evolve: prevent a crash in httpclient_pushobsmarkers() when pushing
>>>>>
>>>>> Pushing to main with an eyes brown raised.
>>>>
>>>> Given the previous code, I can't believe it used to work at all over
>>>> http, even though the test says it does.  Any thoughts?  Any debug  
>>>> steps
>>>> you think would be useful on this repo to see why it is different?
>>>
>>>
>>> The tests are supposed to ensure that. If the http push is actually
>>> untested, making it tested sounds like a first step.
>>
>> I think test-simple4server.t, line 90 covers it:
>>
>>    $ hg push
>>    pushing to http://localhost:$HGPORT/
>>    searching for changes
>>    remote: adding changesets
>>    remote: adding manifests
>>    remote: adding file changes
>>    remote: added 1 changesets with 1 changes to 1 files (+1 heads)
>>    pushing 2 obsolescence markers (* bytes) (glob)
>>
>> I can't believe specifying a path or revision like I did in the commit
>> message demo makes any difference.  The code looked like it would fail
>> any time there was a "pushing xx obsolete markers" line, so I must be
>> missing something.
>
> Your initial message is talking about situation when no changeset are to  
> be pushed. This might be related ?

I see what is going on here.  In the test case, the string that was  
unpacked into "ret, output" was '0\n'.  In my case when there was a  
problem, the string being unpacked is '0\n8 obsolescence markers added'.   
I'll see if I can put together a test case when I get a chance.

--Matt

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -2519,11 +2519,20 @@ 
     (Cannot simply use _callpush as http is doing some special handling)"""
     self.requirecap('_evoext_pushobsmarkers_0',
                     _('push obsolete markers faster'))
-    ret, output = self._call('evoext_pushobsmarkers_0', data=obsfile)
-    for l in output.splitlines(True):
-        if l.strip():
-            self.ui.status(_('remote: '), l)
-    return ret
+    try:
+        r = self._call('evoext_pushobsmarkers_0', data=obsfile)
+        vals = r.split('\n', 1)
+        if len(vals) < 2:
+            raise error.ResponseError(_("unexpected response:"), r)
+
+        for l in vals[1].splitlines(True):
+            if l.strip():
+                self.ui.status(_('remote: '), l)
+        return vals[0]
+    except socket.error, err:
+        if err.args[0] in (errno.ECONNRESET, errno.EPIPE):
+            raise util.Abort(_('push failed: %s') % err.args[1])
+        raise util.Abort(err.args[1])
 
 @eh.wrapfunction(localrepo.localrepository, '_restrictcapabilities')
 def local_pushobsmarker_capabilities(orig, repo, caps):