Arvados

10586-writer-pool-deadlock

test 86f04235021d84afa0d28d105111422e0dd15738

I would feel better if this exit path also called self.pending_tries_notification.notify_all() (or maybe wrap the whole method in a try/finally that calls notify_all()) since spurious wakeups are preferrable to spurious deadlocks.

keep.py:529

                    if self.pending_copies() < 1:
                        # Drain the queue and then raise Queue.Empty
                        while True:
                            self.get_nowait()
                            self.task_done()

This should be if e is not self.TaskFailed ("is" is the object identity operator in Python).

keep.py:598

                except Exception as e:
                    if e != self.TaskFailed:
                        _logger.exception("Exception in KeepWriterThread")

This method is supposed to return a 2-tuple, but if service.finished() it will return None, which will raise an iteration error when it tries to unpack into locator, copies (maybe you want this to raise TaskFailed?)

keep.py:606

        def do_task(self, service, service_root):
            if service.finished():
                return

Peter Amstutz wrote:

I would feel better if this exit path also called self.pending_tries_notification.notify_all() (or maybe wrap the whole method in a try/finally that calls notify_all()) since spurious wakeups are preferrable to spurious deadlocks.

keep.py:529
[...]

It's impossible for any other thread to be stuck in wait() at this point, because write_success() has already called notify_all() after pending_copies() dropped to zero (or it was always zero, in which case no thread could have even called wait()). But I agree that an extra notify_all() is cheap insurance so I've added that.

This should be if e is not self.TaskFailed ("is" is the object identity operator in Python).

keep.py:598
[...]

Fixed.

This method is supposed to return a 2-tuple, but if service.finished() it will return None, which will raise an iteration error when it tries to unpack into locator, copies (maybe you want this to raise TaskFailed?)

keep.py:606
[...]

Indeed. Fixed this by addressing it back in get_next_task(), so we don't pile on more "last result was an error" outcomes after a service starts reporting finished() (which includes "success"!).

e4664336c420836bf26f423faf2af9316302da93

10586-writer-pool-deadlock LGTM @ e4664336c420836bf26f423faf2af9316302da93