This is an archive of the discontinued Mercurial Phabricator instance.

Differential D705

bundle2: move exception handling into part iterator
ClosedPublic

Authored by durham on Sep 13 2017, 8:38 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG21c2df59a1da: bundle2: move exception handling into part iterator
rHG3d8fb0c37e12: bundle2: move exception handling into part iterator
Summary

As part of separating the part iteration logic from the part handling logic,
let's move the exception handling to the part iterator class.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durham created this revision.Sep 13 2017, 8:38 PM
indygreg accepted this revision.Sep 13 2017, 11:28 PM
indygreg added a subscriber: indygreg.
indygreg added inline comments.
mercurial/bundle2.py
387

I'll fix the typo in flight.

This revision is now accepted and ready to land.Sep 13 2017, 11:28 PM
Closed by commit rHG3d8fb0c37e12: bundle2: move exception handling into part iterator (authored by durham). · Explain WhySep 13 2017, 11:30 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Sep 14 2017, 12:26 PM
martinvonz added inline comments.
mercurial/bundle2.py
368

Doesn't look like this can actually happen (if used properly in a with-statement)

402

Looks like a small change in behavior here: the debug message below used to always be printed (before this patch and also before this series). Just pointing it out in case you didn't notice. I'll let you decide if it's worth restoring the message by putting it back in a finally-block.

indygreg added a comment.Sep 25 2017, 4:54 AM

This commit regressed the behavior of ^C during part application (ad41739c6b2b and issue 4784). Before, ^C would abort bundle2 application pretty quickly. Afterwards, it appears we now attempt I/O to seek to the end of the part (or something).

You can test this by running hg clone https://hg.mozilla.org/mozilla-unified and hitting ^C after some changesets are applied. If the process doesn't terminate right away and requires a 2nd ^C to terminate, the behavior is bad.

Revision Contents
Changeset List

PathPackages
Mmercurial/bundle2.py (80 lines)

Diff 1809

mercurial/bundle2.py

return processbundle(repo, unbundler, lambda: tr) return processbundle(repo, unbundler, lambda: tr)
else: else:
# the transactiongetter won't be used, but we might as well set it # the transactiongetter won't be used, but we might as well set it
op = bundleoperation(repo, lambda: tr) op = bundleoperation(repo, lambda: tr)
_processchangegroup(op, unbundler, tr, source, url, **kwargs) _processchangegroup(op, unbundler, tr, source, url, **kwargs)
return op return op
class partiterator(object): class partiterator(object):
def __init__(self, repo, unbundler): def __init__(self, repo, op, unbundler):
self.repo = repo self.repo = repo
self.op = op
self.unbundler = unbundler self.unbundler = unbundler
self.iterator = None self.iterator = None
self.count = 0 self.count = 0
def __enter__(self): def __enter__(self):
def func(): def func():
itr = enumerate(self.unbundler.iterparts()) itr = enumerate(self.unbundler.iterparts())
for count, p in itr: for count, p in itr:
self.count = count self.count = count
yield p yield p
self.iterator = func() self.iterator = func()
return self.iterator return self.iterator
def __exit__(self, type, value, tb): def __exit__(self, type, exc, tb):
if not self.iterator: if not self.iterator:
martinvonzUnsubmitted
Not Done

Doesn't look like this can actually happen (if used properly in a with-statement)

martinvonz: Doesn't look like this can actually happen (if used properly in a with-statement)
return return
if exc:
# Any exceptions seeking to the end of the bundle at this point are
# almost certainly related to the underlying stream being bad.
# And, chances are that the exception we're handling is related to
# getting in that bad state. So, we swallow the seeking error and
# re-raise the original error.
seekerror = False
try:
for part in self.iterator:
# consume the bundle content
part.seek(0, 2)
except Exception:
seekerror = True
# Small hack to let caller code distinguish exceptions from bundle2
# processing from processing the old format. This is mostly needed
# to handle different return codes to unbundle according tothe type
indygregUnsubmitted
Not Done

I'll fix the typo in flight.

indygreg: I'll fix the typo in flight.
# of bundle. We should probably clean up or drop this return code
# craziness in a future version.
exc.duringunbundle2 = True
salvaged = []
replycaps = None
if self.op.reply is not None:
salvaged = self.op.reply.salvageoutput()
replycaps = self.op.reply.capabilities
exc._replycaps = replycaps
exc._bundle2salvagedoutput = salvaged
# Re-raising from a variable loses the original stack. So only use
# that form if we need to.
if seekerror:
raise exc
martinvonzUnsubmitted
Not Done

Looks like a small change in behavior here: the debug message below used to always be printed (before this patch and also before this series). Just pointing it out in case you didn't notice. I'll let you decide if it's worth restoring the message by putting it back in a finally-block.

martinvonz: Looks like a small change in behavior here: the debug message below used to always be printed…
self.repo.ui.debug('bundle2-input-bundle: %i parts total\n' % self.repo.ui.debug('bundle2-input-bundle: %i parts total\n' %
self.count) self.count)
def processbundle(repo, unbundler, transactiongetter=None, op=None): def processbundle(repo, unbundler, transactiongetter=None, op=None):
"""This function process a bundle, apply effect to/from a repo """This function process a bundle, apply effect to/from a repo
It iterates over each part then searches for and uses the proper handling It iterates over each part then searches for and uses the proper handling
code to process the part. Parts are processed in order. code to process the part. Parts are processed in order.
msg.append(' %i params' % len(unbundler.params)) msg.append(' %i params' % len(unbundler.params))
if op._gettransaction is None or op._gettransaction is _notransaction: if op._gettransaction is None or op._gettransaction is _notransaction:
msg.append(' no-transaction') msg.append(' no-transaction')
else: else:
msg.append(' with-transaction') msg.append(' with-transaction')
msg.append('\n') msg.append('\n')
repo.ui.debug(''.join(msg)) repo.ui.debug(''.join(msg))
with partiterator(repo, unbundler) as parts: with partiterator(repo, op, unbundler) as parts:
part = None
try:
for part in parts: for part in parts:
_processpart(op, part) _processpart(op, part)
except Exception as exc:
# Any exceptions seeking to the end of the bundle at this point are
# almost certainly related to the underlying stream being bad.
# And, chances are that the exception we're handling is related to
# getting in that bad state. So, we swallow the seeking error and
# re-raise the original error.
seekerror = False
try:
for part in parts:
# consume the bundle content
part.seek(0, 2)
except Exception:
seekerror = True
# Small hack to let caller code distinguish exceptions from bundle2
# processing from processing the old format. This is mostly needed
# to handle different return codes to unbundle according to the type
# of bundle. We should probably clean up or drop this return code
# craziness in a future version.
exc.duringunbundle2 = True
salvaged = []
replycaps = None
if op.reply is not None:
salvaged = op.reply.salvageoutput()
replycaps = op.reply.capabilities
exc._replycaps = replycaps
exc._bundle2salvagedoutput = salvaged
# Re-raising from a variable loses the original stack. So only use
# that form if we need to.
if seekerror:
raise exc
else:
raise
return op return op
def _processchangegroup(op, cg, tr, source, url, **kwargs): def _processchangegroup(op, cg, tr, source, url, **kwargs):
ret = cg.apply(op.repo, tr, source, url, **kwargs) ret = cg.apply(op.repo, tr, source, url, **kwargs)
op.records.add('changegroup', { op.records.add('changegroup', {
'return': ret, 'return': ret,
}) })