Issue 871

[ashleyheath]

Closes #871

Successful PR Checklist:

• Tests
  • (not applicable?)
• Documentation
  • (not applicable?)

[github-actions]

Coverage report

The coverage rate went from 99.33% to 99.33% ⬆️
The branch rate is 98%.

100% of new lines are covered.

Diff Coverage details (click to unfold)

procrastinate/retry.py

100% of new lines are covered (100% of the complete file).

procrastinate/tasks.py

100% of new lines are covered (100% of the complete file).

procrastinate/worker.py

100% of new lines are covered (100% of the complete file).

[ewjoachim]

A very promising PR :) We're not far from being in a mergeable state. I've included comments, and on a copy of this branch, I've added a commit for how I would have seen it (it's a suggestion more than anything mandatory, but I have the feeling that it's slightly simpler). Feel free to cherry-pick and/or take part of it.

My idea is that we had 2 "failing" outcomes to run_job: fail with retry and fail without retry (2 exceptions). We now have 4 outcomes: critical/non-critical fail with/without retry. This smells a lot like the "inheritence vs composition debate", and indicates (to me) that the difference between retry and fail should be a difference of an attribute, not of a subtype. Sadly, we can't easily change RetryStrategy.get_retry_exception's return type because that's part of the public API (though we could change Task.get_retry_exception's signature, might be better that what I did here).

9f5e5be

[ewjoachim]

I though that throughout the whole codebase, all the exceptions were always named exc. It turns out there are a handful of exceptions, but I'd rather we try and keep it that way (you don't have to fix the other non-exc exceptions, of course, but it's perfectly fine if you choose to do it, otherwise, I might or might not do it later)

Note

this whole function is removed in the commit I suggested

[ewjoachim]

(should we also test run_job when the job raises a BaseException ? That was you original bug)

[ewjoachim]

I'm personally extremely wary about the idea of raising or returning from within a finally: block. This is one of the lesser know python gotchas that can bite very hard.

try:
	0/0
except DivisionError:
	raise ValueError
finally:
	return True

The example above is making the ValueError exception disappear completely. Poof. Nobody can except it. Gone with the wind (same happens if you raise from within the finally).
Given that "Errors should never pass silently", I've always considered this a very dangerous practice.

For example: imagine I add the following log line:

        try:
            await self.run_job(job=job, worker_id=worker_id)
            status = jobs.Status.SUCCEEDED
        except exceptions.JobError as e:
            status = jobs.Status.FAILED
            exception_to_re_raise = find_exception_to_re_raise(e)
            logger.errorr("Ooops, something went bad")
        finally:
			# ...
            if exception_to_re_raise is not None:
                raise exception_to_re_raise

I would never find my log, though the rest would work. Because I typed errorr instead of error, the code produces: AttributeError: 'RootLogger' object has no attribute 'errorr'. Did you mean: 'error'? but that exception vanishes when we raise in the finally. I'll never know that my code has a bug.

What you really want to do is raise in the except: block. It will actually do the raising after the finally block is executed.

Note

I took care of this in the commit I suggested

[ewjoachim]

I've added a commit for taking care of tests in my branch (https://github.com/procrastinate-org/procrastinate/commits/issue-871-ewjoachim/) (both fixing what my previous commit has broken, and adding the test I mentioned).

As it stands, I'd be ready to merge our 2 commits together. But this is your PR, so I'll wait for you opinion before doing so :) And rest assured it's perfectly ok to not agree with my changes!

[ashleyheath]

Morning @ewjoachim! Your changes make perfect sense; very happy for you to merge with your additions