Allow force-quitting the pipeline

[wisp3rwind]

The situation: I was running an import task, that apparently locked up, i.e. it didn't advance for hours, even though not all user queries were run. Replaygain was enabled, but for 20-ish albums, even that should not take as long. Worse, I couldn't quit beets per KeyboardInterrupt and as a result didn't end up with a traceback when I eventually kill-ed beets. Not very satisfying, because I couldn't find out where it actually got stuck.

(The actual culprit may have been related to a flaky USB connection to the external drive with the music.)

Therefore, this is my attempt at allowing to quit at any time, safeguarded by having to press Ctrl-C rapidly several times (and a warning about data loss). What do you think? Worth merging, or better off as a developer tool in a separate branch?

I think at least the first commit should go in, as it fixes a few catch-all except:s. According to grep, the only remaining catch-alls are now in beets/util/bluelet.py, which I dared not touch. Also, it is only used by the bpd plugin (again, says grep), and might already be properly handled (I did not read the bluelet code).

This tackles #803 , but does not actually solve the problem (Maybe I'll revive that issue).

[codecov-io]

Current coverage is 76.86%

Merging #1957 into master will increase coverage by +<.01%

@@             master      #1957   diff @@
==========================================
  Files            62         62          
  Lines         12900      12919    +19   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9913       9929    +16   
- Misses         2987       2990     +3   
  Partials          0          0          

Powered by Codecov. Last updated by 221df29

[jackwilsdon]

Well that's new. Could they have made the graph any bigger? 😆

[sampsyo]

Oh, that's terrible. I definitely didn't turn that on—I find those code coverage notifications uselessly noisy. Let me see if there's some setting I need to turn off again…

[sampsyo]

OK, I hope I've turned that off. It looks like CodeCov changed the way they do configuration without notifying us. 😢 I had to create a new codecov.yaml in the repository; hopefully that does the trick.

[sampsyo]

This (and _run in the subclasses) could use some docstrings to explain their new relationship.

[sampsyo]

Interesting! Yes, the inability to ^C is a frustrating aspect of the multithreaded importer. We should look at this carefully.

• The "expunge untyped except" change is definitely a good idea. Perhaps we should even add a style checker to enforce this rule. We can probably even get this right for Bluelet—you're right that this is old and very scary code, but these days it's only used for BPD (as you've noted), which has generally fallen out of favor anyway. Feel free to cherry-pick that commit onto master anytime.
• A "hard abort" might be useful for developers, but we should probably also look into what exactly is causing an uninterruptible process. Maybe we can blame exactly what was taking so long and find a way to break it up before resorting to this workaround. If not, though, a double- or triple-^C might indeed be the right interface? It would be great to leave as light a touch as possible on the pipeline stuff, though, which is rather complex but—at the moment—well-tested and separate from the rest of beets.

Of course, the best thing would be some magical way to raise KeyboardInterrupt in all threads "simultaneously" so all the finally: handlers run. But this may be impossible in Python. 😢

[wisp3rwind]

@sampsyo: I guess this should rather be for thread in threads:? For now I didn't change the slice, but to my understanding, threads[-1] is only guaranteed to have finished upon normal shutdown, when aborting, it might very well be alive.

[sampsyo]

Hmm, maybe so. Perhaps this is a case where "try it and see" would be a good idea?

[wisp3rwind]

I'm currently importing some music with that patch, seems fine for now. As long as the threads are non-daemonized, this might not make any difference. But as the last thread (I think) will do the actual writing, it possibly is one of the more important to wait for.

[wisp3rwind]

Seems sensible. I'll do the cherrypick, and leave the remaining code here without merging.

There's https://github.com/elijahandrews/flake8-blind-except, flake8 does not seem to include a checker like this by default.

Of course, the best thing would be some magical way to raise KeyboardInterrupt in all threads "simultaneously" so all the finally: handlers run. But this may be impossible in Python. 😢

Some searching seems to confirm this, although the docs on threading in general are not that good. Thanks for the heads-up on finally handlers, didn't think about them.

But the better way should be using the proposals form #803, anyway.
Something like combination a of:

def check_abort():
  t = threading.current_thread()
  if t.abort_flag:
    raise PipelineAbort()

and

# read t.abort_stack[-1] when aborting the pipeline to see whether thread t needs to be waited for
@contextmanager
def can_abort(allow_abort):
  t = threading.current_thread
  t.abort_stack.append(allow_abort)
  yield
  t.abort_stack.pop()

whatever is better suited for the specific thread. Then make the threads use it. Maybe add some profiling code and check on some real import tasks what actually takes a long time.

[sampsyo]

An optional profiling mode, to see where the queues are backed up and such, would be incredibly cool for many reasons.

[wisp3rwind]

For reference, beets probably did not really lock up at all, but ran into this ntfs-3g issue: http://tuxera.com/forum/viewtopic.php?f=2&t=31065 when mutagen resizes the metadata block using mmap. Which is rather unfortunate, as it gives a huge performance drop.

[jtpavlock]

Hello @wisp3rwind and thank you for your contribution. I know it's been some time, but are you still intending to continue work on this?

[wisp3rwind]

I still care for improved pipeline aborting, however, this PR can be closed: A part of it (blind except removal) has been implemented in #1959, and the rest should be split into some generic pipeline refactoring without functional changes, and the actual abort code. If I ever get around to this, I'll open new, more focused PRs.