verigak / progress

Easy to use progress bars for Python
ISC License
1.41k stars 179 forks source link

SigIntMixin does not allow easy recovery after an interruption #20

Closed PiDelport closed 8 years ago

PiDelport commented 9 years ago

I recently tried using this library's SigIntMixin to resolve pip issue #2462, but it turns out that SigIntMixin does not provide quite enough functionality to work cleanly in that context.

For pip, I ended up implementing a very similar InterruptibleMixin class: see its docstring for discussion.

The primary differences between the two are that InterruptibleMixin:

  1. Does not raise SystemExit
  2. Only installs its signal handler for the duration of the progress bar display
  3. Delegates to the original signal handler on interruption (and restores it afterward)

This means that InterruptibleMixin should cooperate with any SIGINT handler the calling context already has in place: in particular, it cooperates with Python's default handler and propagates its KeyboardInterrupt, which pip's existing code handles more easily. (Other programs could catch and recover from it, to let the user cancel a download and continue with something else, for example.)

Discussion: context managers?

With all the above said, I am filing this issue mainly to spark discussion: although something like InterruptibleMixin works better for pip and maybe other programs, I am not sure that it is the best way of dealing with this problem in general.

The primary need for a mixin like this is simply to reliably call contextual cleanup code (like the cursor hiding and unhiding mixins), which Python already has a more robust solution for in the form of context managers.

If all the progress bar display setup and cleanup operations were implemented in terms of the context manager API, then there should be no need for comparative hacks like SIGINT handlers at all: the code would do the right thing for KeyboardInterrupt as well as any other exceptions that cross the context boundary.

verigak commented 8 years ago

I basically agree with everything you wrote. The purpose of SigIntMixin was to provide an example of mixins that could be used with progress bars, that's why it is not used anywhere else. However if you would like to send a pull request with your implementation I'd be happy to merge as it is indeed more general and could be useful for others.