This is an archive of the discontinued Mercurial Phabricator instance.

Differential D960

bundle2: immediate exit for ctrl+c (issue5692)
ClosedPublic

Authored by durham on Oct 5 2017, 5:20 PM.

Details

Reviewers
quark
Group Reviewers
hg-reviewers
Commits
rHG5f79f5f8487a: bundle2: immediate exit for ctrl+c (issue5692)
Summary

21c2df59a regressed bundle2 by catching all exceptions and trying to handle
them. The old behavior was to allow KeyboardInterrupts to throw and not have
graceful cleanup, which allowed it to exit immediately. Let's go back to that
behavior.

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.Oct 5 2017, 5:20 PM
quark accepted this revision.Oct 5 2017, 6:31 PM
quark requested changes to this revision.
quark added inline comments.
mercurial/bundle2.py
372–373

Actually KeyboardInterrupt is tested here. So there is something else going on. Maybe we should add error.SignalInterrupt to the list?

This revision now requires changes to proceed.Oct 5 2017, 6:32 PM
yuja added a subscriber: yuja.Oct 6 2017, 11:05 AM
yuja added inline comments.
mercurial/bundle2.py
372–373

SignalInterrupt is a subclass of KeyboardInterrupt, so there might
be deeper problem.

FWIW, I think it's probably a good idea to replace this blacklist
with isinstance(exc, Exception).

durham added inline comments.Oct 11 2017, 1:33 PM
mercurial/bundle2.py
372–373

Before my refactor there used to be two levels of error handling, 1) inside _parthandler which tried to seek to the end if it wasn't systemexit or keyboardinterrup, and 2) inside processbundle (which calls _parthandler) which only caught type Exception.

It seems like the correct unification of these would be to just keep the isinstance(exc, Exception) check above, and delete this sub-check entirely. I'll send an update.

durham updated this revision to Diff 2592.Oct 11 2017, 1:44 PM
durham marked 3 inline comments as done.Oct 11 2017, 1:44 PM
quark accepted this revision.Oct 11 2017, 5:36 PM
Closed by commit rHG5f79f5f8487a: bundle2: immediate exit for ctrl+c (issue5692) (authored by durham). · Explain WhyOct 13 2017, 10:05 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/bundle2.py (16 lines)

Diff 2675

mercurial/bundle2.py

p.seek(0, 2) p.seek(0, 2)
self.current = None self.current = None
self.iterator = func() self.iterator = func()
return self.iterator return self.iterator
def __exit__(self, type, exc, tb): def __exit__(self, type, exc, tb):
if not self.iterator: if not self.iterator:
return return
if exc: # Only gracefully abort in a normal exception situation. User aborts
quarkUnsubmitted
Done

Actually KeyboardInterrupt is tested here. So there is something else going on. Maybe we should add error.SignalInterrupt to the list?

quark: Actually `KeyboardInterrupt` is tested here. So there is something else going on. Maybe we…
yujaUnsubmitted
Done

SignalInterrupt is a subclass of KeyboardInterrupt, so there might
be deeper problem.

FWIW, I think it's probably a good idea to replace this blacklist
with isinstance(exc, Exception).

yuja: SignalInterrupt is a subclass of KeyboardInterrupt, so there might be deeper problem. FWIW, I…
durhamAuthorUnsubmitted
Done

Before my refactor there used to be two levels of error handling, 1) inside _parthandler which tried to seek to the end if it wasn't systemexit or keyboardinterrup, and 2) inside processbundle (which calls _parthandler) which only caught type Exception.

It seems like the correct unification of these would be to just keep the isinstance(exc, Exception) check above, and delete this sub-check entirely. I'll send an update.

durham: Before my refactor there used to be two levels of error handling, 1) inside _parthandler which…
# If exiting or interrupted, do not attempt to seek the stream in # like Ctrl+C throw a KeyboardInterrupt which is not a base Exception,
# the finally block below. This makes abort faster. # and should not gracefully cleanup.
if (self.current and if isinstance(exc, Exception):
not isinstance(exc, (SystemExit, KeyboardInterrupt))):
# consume the part content to not corrupt the stream.
self.current.seek(0, 2)
# Any exceptions seeking to the end of the bundle at this point are # Any exceptions seeking to the end of the bundle at this point are
# almost certainly related to the underlying stream being bad. # almost certainly related to the underlying stream being bad.
# And, chances are that the exception we're handling is related to # And, chances are that the exception we're handling is related to
# getting in that bad state. So, we swallow the seeking error and # getting in that bad state. So, we swallow the seeking error and
# re-raise the original error. # re-raise the original error.
seekerror = False seekerror = False
try: try:
if self.current:
# consume the part content to not corrupt the stream.
self.current.seek(0, 2)
for part in self.iterator: for part in self.iterator:
# consume the bundle content # consume the bundle content
part.seek(0, 2) part.seek(0, 2)
except Exception: except Exception:
seekerror = True seekerror = True
# Small hack to let caller code distinguish exceptions from bundle2 # Small hack to let caller code distinguish exceptions from bundle2
# processing from processing the old format. This is mostly needed # processing from processing the old format. This is mostly needed