Add support for abort notification

[onlyann]

trafficstars

Successful PR Checklist:

• [ ] Tests
  • [ ] (not applicable?)
• [x] Documentation
  • [ ] (not applicable?)

PR label(s):

• [x] https://github.com/procrastinate-org/procrastinate/labels/PR%20type%3A%20breaking%20%F0%9F%92%A5
• [x] https://github.com/procrastinate-org/procrastinate/labels/PR%20type%3A%20feature%20%E2%AD%90%EF%B8%8F
• [ ] https://github.com/procrastinate-org/procrastinate/labels/PR%20type%3A%20bugfix%20%F0%9F%95%B5%EF%B8%8F
• [ ] https://github.com/procrastinate-org/procrastinate/labels/PR%20type%3A%20miscellaneous%20%F0%9F%91%BE
• [ ] https://github.com/procrastinate-org/procrastinate/labels/PR%20type%3A%20dependencies%20%F0%9F%A4%96
• [ ] https://github.com/procrastinate-org/procrastinate/labels/PR%20type%3A%20documentation%20%F0%9F%93%9A

Context

So far, the only use of LISTEN/NOTIFY has been to let the worker know that a new job is ready to be processed.

This extends listen_notify to accept different types of notification through using the payload:

• job_inserted: the existing notification
• abort_job_requested: when a running job is requested to be aborted.

Instead of making explicit calls to the database every time a job calls should_abort, the worker uses the same mechanism than for detecting new jobs, a combination of Postgres notification and polling.

Other changes

The edge case of retrying a job to be aborted is handled at the database level instead of the worker.

Next steps

Once this change lands in v3, it will help with cancellation during the shutdown event and #1084

[github-actions[bot]]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
procrastinate
connector.py
job_context.py
jobs.py
manager.py
psycopg_connector.py
testing.py
utils.py
worker.py					363-364
procrastinate/contrib/aiopg
aiopg_connector.py
Project Total

This report was generated by python-coverage-comment-action

[medihack]

Excellent. I will have a look at it tomorrow 🙂. I think it's a good idea to handle more stuff at the database level (as long as we can still test it).

[onlyann]

Excellent. I will have a look at it tomorrow 🙂. I think it's a good idea to handle more stuff at the database level (as long as we can still test it).

It also removes some race condition with the previous approach.

Glad you had an acceptance test written against it already or I will have surely missed it.

[medihack]

Excellent. I will have a look at it tomorrow 🙂. I think it's a good idea to handle more stuff at the database level (as long as we can still test it).

It also removes some race condition with the previous approach.

Glad you had an acceptance test written against it already or I will have surely missed it.

Cool, let me know when it's ready for review (still a draft currently and some connector excluded from tests/acceptance/test_async.py). But looks pretty good at first glance.

[ewjoachim]

coverage is down but it's not your fault. I'll fix it and you'll need to rebase.

[ewjoachim]

https://github.com/procrastinate-org/procrastinate/pull/1179 PR should be merged quickly. Sorry for the mess (it's all GitHub's fault :D )

[onlyann]

#1179 PR should be merged quickly. Sorry for the mess (it's all GitHub's fault :D )

No worries. I can rebase once it lands in v3 branch

[ewjoachim]

v3 is up to date :)

[onlyann]

Rebased and squashed All 🟢 🥳

[ewjoachim]

And Merged !