RFE: task.py clarity

[Michael Schwendt]

A few thoughts while I reviewed task.py and the classes that are derived from it:

        self.paused = False

is set in two places, but neither BackgroundTask itself nor the derived TaskQueue uses it. The first time it's used is in ConverterQueue(TaskQueue), and now it gets interesting. If the ConverterQueue is paused, it does not copy the state to the individual tasks. For each task, "paused" remains False, but instead the converter queue calls toggle_pause() for each task, which is a method only provided by Pipeline. I would expect the underlying BackgroundTask to reflect the "paused" state even if its default implementation doesn't evaluate it anywhere.

        self.progress = None

is similar. Except that TaskQueue updates this value actually. But with a float (or 0), so the default "None" is overwritten early and else it only makes it more troublesome to use this value. Indeed, ui.py would prefer a default of "0" already, too, and works around the "None": progress = queue.progress if queue.progress else 0

      def abort(self):

is overridden by TaskQueue and ConverterQueue, but only Pipeline(BackgroundTask) introduces a self.aborted flag not available in the class it is derived from. TaskQueue.abort() aborts each running task, but nothing sets an "aborted" flag for the tasks. With nothing other than the Pipeline evaluating such an "aborted" flag, there might be a rarely encountered race condition (perhaps when cancelling and restarting conversion) given that task.done() emits a "finished" idle callback, which can reach the queue's task_finished() method *after* the queue tried to abort running tasks.

What happens in a decoder pipeline task? Whereas BackgroundTask.abort() says "finished() is not called" in a comment, Pipeline.abort() calls its own finished(). An unfortunate name for that method.

[GautierPortet (kassoulet)]

You are right, I remove the unused Task.paused.

I don't see the problem with progress. Progress is None before being at an allowed value. So we know.
Well, it's not used, but whatever :)

Oh, sorry for the confusion with abort/finish. I've overlooked the whole thing.
Check if this is better: https://github.com/kassoulet/soundconverter/commit/a0896c4a992f683ed59e0df79ad002f3a44bed37

[Michael Schwendt (mschwendt)]

Sorry for not getting to this as fast as I'd like. The original problem [an aborted task reaching task_finished()] is hard to reproduce and might be some sort of race. I currently protect against it with this in the queue, just for safety reasons:

- self.running_tasks.remove(task)
- self.finished_tasks += 1
- self.start_next_task()
+ # only remove known tasks and then start the next one,
+ # this shall avoid issues with an aborted task reaching finished()
+ # via done()
+ if task in self.running_tasks:
+ self.running_tasks.remove(task)
+ self.finished_tasks += 1
+ self.start_next_task()

And yeah, the commit clears up some of the confusion. Anything that makes the derivation (with overloading) less ambiguous will be an improvement, so e.g. task state would be reflected by the task objects properly, and the task queue could handle (= recognize) aborted and paused tasks without relying on only its own arrays, such as running_tasks, waiting_tasks and the finished_tasks count.

[GautierPortet (kassoulet)]

And... you are right again!
Fixed in: https://github.com/kassoulet/soundconverter/commit/28d4ce6b8946fa0b197984eaeb8d850bdb1b92bc

[Michael Schwendt (mschwendt)]

Remains the question whether such an aborted task (that is no longer on the running_tasks list) should increment finished_tasks and make the queue start the next task? I didn't realize the quoted patch has no indentation.