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
Lint Skipped
Unit
Unit Tests Skipped

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
380

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
380

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
380

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 (5 lines)

Diff 2476

mercurial/bundle2.py

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
# like Ctrl+C throw a KeyboardInterrupt which is not a base Exception,
# and should not gracefully cleanup.
if isinstance(exc, Exception):
# If exiting or interrupted, do not attempt to seek the stream in # If exiting or interrupted, do not attempt to seek the stream in
# the finally block below. This makes abort faster. # the finally block below. This makes abort faster.
if (self.current and if (self.current and
not isinstance(exc, (SystemExit, KeyboardInterrupt))): not isinstance(exc, (SystemExit, KeyboardInterrupt))):
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…
# consume the part content to not corrupt the stream. # consume the part content to not corrupt the stream.
self.current.seek(0, 2) 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.