Patchwork [2,of,2] bundle2: transmit exception during part generation

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 21, 2014, 8:08 p.m.
Message ID <bd35fb4708eda7d548a3.1413922126@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/6437/
State Accepted
Headers show

Comments

Pierre-Yves David - Oct. 21, 2014, 8:08 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1413370340 25200
#      Wed Oct 15 03:52:20 2014 -0700
# Branch stable
# Node ID bd35fb4708eda7d548a34ef6ff504505cf0e85cb
# Parent  1c1b98ea77615d601d8234f70728da9a21692c93
bundle2: transmit exception during part generation

If an exception is raised during a bundle2 part payload generation it is now
recorded in the bundle. If such exception occurs, we capture it, transmit an
abort exception through the bundle, cleanly close the current part payload and
raise it again. This allow to generate valid bundle even in case of exception so
that the consumer does not wait forever for a dead producer. This also allow to
raise the exception during unbundling at the exact point it happened during
bundling make debugging easier.
Augie Fackler - Oct. 22, 2014, 2:46 a.m.
On Tue, Oct 21, 2014 at 01:08:46PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1413370340 25200
> #      Wed Oct 15 03:52:20 2014 -0700
> # Branch stable
> # Node ID bd35fb4708eda7d548a34ef6ff504505cf0e85cb
> # Parent  1c1b98ea77615d601d8234f70728da9a21692c93
> bundle2: transmit exception during part generation

queued these for stable as well

>
> If an exception is raised during a bundle2 part payload generation it is now
> recorded in the bundle. If such exception occurs, we capture it, transmit an
> abort exception through the bundle, cleanly close the current part payload and
> raise it again. This allow to generate valid bundle even in case of exception so
> that the consumer does not wait forever for a dead producer. This also allow to
> raise the exception during unbundling at the exact point it happened during
> bundling make debugging easier.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -143,10 +143,11 @@ process is aborted, the full bundle is s
>  channel usable. But none of the part read from an abort are processed. In the
>  future, dropping the stream may become an option for channel we do not care to
>  preserve.
>  """
>
> +import sys
>  import util
>  import struct
>  import urllib
>  import string
>  import obsolete
> @@ -671,13 +672,26 @@ class bundlepart(object):
>          ## finalize header
>          headerchunk = ''.join(header)
>          yield _pack(_fpartheadersize, len(headerchunk))
>          yield headerchunk
>          ## payload
> -        for chunk in self._payloadchunks():
> -            yield _pack(_fpayloadsize, len(chunk))
> -            yield chunk
> +        try:
> +            for chunk in self._payloadchunks():
> +                yield _pack(_fpayloadsize, len(chunk))
> +                yield chunk
> +        except Exception:
> +            # backup exception data for later
> +            exc_info = sys.exc_info()
> +            msg = 'unexpected error: %s' % exc
> +            interpart = bundlepart('b2x:error:abort', [('message', msg)])
> +            interpart.id = 0
> +            yield _pack(_fpayloadsize, -1)
> +            for chunk in interpart.getchunks():
> +                yield chunk
> +            # abort current part payload
> +            yield _pack(_fpayloadsize, 0)
> +            raise exc_info[0], exc_info[1], exc_info[2]
>          # end of payload
>          yield _pack(_fpayloadsize, 0)
>          self._generated = True
>
>      def _payloadchunks(self):
> diff --git a/tests/test-bundle2-format.t b/tests/test-bundle2-format.t
> --- a/tests/test-bundle2-format.t
> +++ b/tests/test-bundle2-format.t
> @@ -775,28 +775,25 @@ with reply
>    added 0 changesets with 0 changes to 3 files
>    \x00\x00\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
>
>  Check handling of exception during generation.
>  ----------------------------------------------
> -(is currently not right)
>
>    $ hg bundle2 --genraise > ../genfailed.hg2
>    abort: Someone set up us the bomb!
>    [255]
>
>  Should still be a valid bundle
> -(is currently not right)
>
>    $ cat ../genfailed.hg2
>    HG2Y\x00\x00\x00\x00\x00\x00\x00\x11 (esc)
> -  b2x:output\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
> +  b2x:output\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff\x00\x00\x00L\x0fb2x:error:abort\x00\x00\x00\x00\x01\x00\x07-messageunexpected error: Someone set us up the bomb!\x00\x00\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
>
>  And its handling on the other size raise a clean exception
> -(is currently not right)
>
>    $ cat ../genfailed.hg2 | hg unbundle2
>    0 unread bytes
> -  abort: stream ended unexpectedly (got 0 bytes, expected 4)
> +  abort: unexpected error: Someone set us up the bomb!
>    [255]
>
>
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Oct. 22, 2014, 5:41 a.m.
On Tue, Oct 21, 2014 at 01:08:46PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1413370340 25200
> #      Wed Oct 15 03:52:20 2014 -0700
> # Branch stable
> # Node ID bd35fb4708eda7d548a34ef6ff504505cf0e85cb
> # Parent  1c1b98ea77615d601d8234f70728da9a21692c93
> bundle2: transmit exception during part generation
>
> If an exception is raised during a bundle2 part payload generation it is now
> recorded in the bundle. If such exception occurs, we capture it, transmit an
> abort exception through the bundle, cleanly close the current part payload and
> raise it again. This allow to generate valid bundle even in case of exception so
> that the consumer does not wait forever for a dead producer. This also allow to
> raise the exception during unbundling at the exact point it happened during
> bundling make debugging easier.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -143,10 +143,11 @@ process is aborted, the full bundle is s
>  channel usable. But none of the part read from an abort are processed. In the
>  future, dropping the stream may become an option for channel we do not care to
>  preserve.
>  """
>
> +import sys
>  import util
>  import struct
>  import urllib
>  import string
>  import obsolete
> @@ -671,13 +672,26 @@ class bundlepart(object):
>          ## finalize header
>          headerchunk = ''.join(header)
>          yield _pack(_fpartheadersize, len(headerchunk))
>          yield headerchunk
>          ## payload
> -        for chunk in self._payloadchunks():
> -            yield _pack(_fpayloadsize, len(chunk))
> -            yield chunk
> +        try:
> +            for chunk in self._payloadchunks():
> +                yield _pack(_fpayloadsize, len(chunk))
> +                yield chunk
> +        except Exception:
> +            # backup exception data for later
> +            exc_info = sys.exc_info()
> +            msg = 'unexpected error: %s' % exc

exc isn't defined here - did you mean exc_info, or did you want to
catch the exception as exc?

> +            interpart = bundlepart('b2x:error:abort', [('message', msg)])
> +            interpart.id = 0
> +            yield _pack(_fpayloadsize, -1)
> +            for chunk in interpart.getchunks():
> +                yield chunk
> +            # abort current part payload
> +            yield _pack(_fpayloadsize, 0)
> +            raise exc_info[0], exc_info[1], exc_info[2]
>          # end of payload
>          yield _pack(_fpayloadsize, 0)
>          self._generated = True
>
>      def _payloadchunks(self):
> diff --git a/tests/test-bundle2-format.t b/tests/test-bundle2-format.t
> --- a/tests/test-bundle2-format.t
> +++ b/tests/test-bundle2-format.t
> @@ -775,28 +775,25 @@ with reply
>    added 0 changesets with 0 changes to 3 files
>    \x00\x00\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
>
>  Check handling of exception during generation.
>  ----------------------------------------------
> -(is currently not right)
>
>    $ hg bundle2 --genraise > ../genfailed.hg2
>    abort: Someone set up us the bomb!
>    [255]
>
>  Should still be a valid bundle
> -(is currently not right)
>
>    $ cat ../genfailed.hg2
>    HG2Y\x00\x00\x00\x00\x00\x00\x00\x11 (esc)
> -  b2x:output\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
> +  b2x:output\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff\x00\x00\x00L\x0fb2x:error:abort\x00\x00\x00\x00\x01\x00\x07-messageunexpected error: Someone set us up the bomb!\x00\x00\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
>
>  And its handling on the other size raise a clean exception
> -(is currently not right)
>
>    $ cat ../genfailed.hg2 | hg unbundle2
>    0 unread bytes
> -  abort: stream ended unexpectedly (got 0 bytes, expected 4)
> +  abort: unexpected error: Someone set us up the bomb!
>    [255]
>
>
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 22, 2014, 6:03 a.m.
On 10/21/2014 10:41 PM, Augie Fackler wrote:
> On Tue, Oct 21, 2014 at 01:08:46PM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1413370340 25200
>> #      Wed Oct 15 03:52:20 2014 -0700
>> # Branch stable
>> # Node ID bd35fb4708eda7d548a34ef6ff504505cf0e85cb
>> # Parent  1c1b98ea77615d601d8234f70728da9a21692c93
>> bundle2: transmit exception during part generation
>>
>> If an exception is raised during a bundle2 part payload generation it is now
>> recorded in the bundle. If such exception occurs, we capture it, transmit an
>> abort exception through the bundle, cleanly close the current part payload and
>> raise it again. This allow to generate valid bundle even in case of exception so
>> that the consumer does not wait forever for a dead producer. This also allow to
>> raise the exception during unbundling at the exact point it happened during
>> bundling make debugging easier.
>>
>> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>> --- a/mercurial/bundle2.py
>> +++ b/mercurial/bundle2.py
>> @@ -143,10 +143,11 @@ process is aborted, the full bundle is s
>>   channel usable. But none of the part read from an abort are processed. In the
>>   future, dropping the stream may become an option for channel we do not care to
>>   preserve.
>>   """
>>
>> +import sys
>>   import util
>>   import struct
>>   import urllib
>>   import string
>>   import obsolete
>> @@ -671,13 +672,26 @@ class bundlepart(object):
>>           ## finalize header
>>           headerchunk = ''.join(header)
>>           yield _pack(_fpartheadersize, len(headerchunk))
>>           yield headerchunk
>>           ## payload
>> -        for chunk in self._payloadchunks():
>> -            yield _pack(_fpayloadsize, len(chunk))
>> -            yield chunk
>> +        try:
>> +            for chunk in self._payloadchunks():
>> +                yield _pack(_fpayloadsize, len(chunk))
>> +                yield chunk
>> +        except Exception:
>> +            # backup exception data for later
>> +            exc_info = sys.exc_info()
>> +            msg = 'unexpected error: %s' % exc
>
> exc isn't defined here - did you mean exc_info, or did you want to
> catch the exception as exc?

actually msg is not used. Drop the line.
Augie Fackler - Oct. 22, 2014, 3:02 p.m.
On Oct 21, 2014, at 11:03 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 
> 
> On 10/21/2014 10:41 PM, Augie Fackler wrote:
>> On Tue, Oct 21, 2014 at 01:08:46PM -0700, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1413370340 25200
>>> #      Wed Oct 15 03:52:20 2014 -0700
>>> # Branch stable
>>> # Node ID bd35fb4708eda7d548a34ef6ff504505cf0e85cb
>>> # Parent  1c1b98ea77615d601d8234f70728da9a21692c93
>>> bundle2: transmit exception during part generation
>>> 
>>> If an exception is raised during a bundle2 part payload generation it is now
>>> recorded in the bundle. If such exception occurs, we capture it, transmit an
>>> abort exception through the bundle, cleanly close the current part payload and
>>> raise it again. This allow to generate valid bundle even in case of exception so
>>> that the consumer does not wait forever for a dead producer. This also allow to
>>> raise the exception during unbundling at the exact point it happened during
>>> bundling make debugging easier.
>>> 
>>> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>>> --- a/mercurial/bundle2.py
>>> +++ b/mercurial/bundle2.py
>>> @@ -143,10 +143,11 @@ process is aborted, the full bundle is s
>>>  channel usable. But none of the part read from an abort are processed. In the
>>>  future, dropping the stream may become an option for channel we do not care to
>>>  preserve.
>>>  """
>>> 
>>> +import sys
>>>  import util
>>>  import struct
>>>  import urllib
>>>  import string
>>>  import obsolete
>>> @@ -671,13 +672,26 @@ class bundlepart(object):
>>>          ## finalize header
>>>          headerchunk = ''.join(header)
>>>          yield _pack(_fpartheadersize, len(headerchunk))
>>>          yield headerchunk
>>>          ## payload
>>> -        for chunk in self._payloadchunks():
>>> -            yield _pack(_fpayloadsize, len(chunk))
>>> -            yield chunk
>>> +        try:
>>> +            for chunk in self._payloadchunks():
>>> +                yield _pack(_fpayloadsize, len(chunk))
>>> +                yield chunk
>>> +        except Exception:
>>> +            # backup exception data for later
>>> +            exc_info = sys.exc_info()
>>> +            msg = 'unexpected error: %s' % exc
>> 
>> exc isn't defined here - did you mean exc_info, or did you want to
>> catch the exception as exc?
> 
> actually msg is not used. Drop the line.

It appears to be on the next line - it looks like you meant to catch Exception as exc, so I've done that and will push soon.

> 
> -- 
> Pierre-Yves David

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -143,10 +143,11 @@  process is aborted, the full bundle is s
 channel usable. But none of the part read from an abort are processed. In the
 future, dropping the stream may become an option for channel we do not care to
 preserve.
 """
 
+import sys
 import util
 import struct
 import urllib
 import string
 import obsolete
@@ -671,13 +672,26 @@  class bundlepart(object):
         ## finalize header
         headerchunk = ''.join(header)
         yield _pack(_fpartheadersize, len(headerchunk))
         yield headerchunk
         ## payload
-        for chunk in self._payloadchunks():
-            yield _pack(_fpayloadsize, len(chunk))
-            yield chunk
+        try:
+            for chunk in self._payloadchunks():
+                yield _pack(_fpayloadsize, len(chunk))
+                yield chunk
+        except Exception:
+            # backup exception data for later
+            exc_info = sys.exc_info()
+            msg = 'unexpected error: %s' % exc
+            interpart = bundlepart('b2x:error:abort', [('message', msg)])
+            interpart.id = 0
+            yield _pack(_fpayloadsize, -1)
+            for chunk in interpart.getchunks():
+                yield chunk
+            # abort current part payload
+            yield _pack(_fpayloadsize, 0)
+            raise exc_info[0], exc_info[1], exc_info[2]
         # end of payload
         yield _pack(_fpayloadsize, 0)
         self._generated = True
 
     def _payloadchunks(self):
diff --git a/tests/test-bundle2-format.t b/tests/test-bundle2-format.t
--- a/tests/test-bundle2-format.t
+++ b/tests/test-bundle2-format.t
@@ -775,28 +775,25 @@  with reply
   added 0 changesets with 0 changes to 3 files
   \x00\x00\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
 
 Check handling of exception during generation.
 ----------------------------------------------
-(is currently not right)
 
   $ hg bundle2 --genraise > ../genfailed.hg2
   abort: Someone set up us the bomb!
   [255]
 
 Should still be a valid bundle
-(is currently not right)
 
   $ cat ../genfailed.hg2
   HG2Y\x00\x00\x00\x00\x00\x00\x00\x11 (esc)
-  b2x:output\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
+  b2x:output\x00\x00\x00\x00\x00\x00\xff\xff\xff\xff\x00\x00\x00L\x0fb2x:error:abort\x00\x00\x00\x00\x01\x00\x07-messageunexpected error: Someone set us up the bomb!\x00\x00\x00\x00\x00\x00\x00\x00 (no-eol) (esc)
 
 And its handling on the other size raise a clean exception
-(is currently not right)
 
   $ cat ../genfailed.hg2 | hg unbundle2
   0 unread bytes
-  abort: stream ended unexpectedly (got 0 bytes, expected 4)
+  abort: unexpected error: Someone set us up the bomb!
   [255]
 
 
   $ cd ..