Refactor towards using a database mutex

[jrief]

Hi @selwin
thanks for merging the last pull request, it emphasizes how important locking is! However using a file for locking, in my opinion, is not a good option anymore. With the advent of Docker/Kubernetes, very often the web application and the Celery worker run on different containers, without sharing a volume which could be used for keeping that lockfile. It also makes the handling of such a lockfile difficult, because it usually is created with a temporary name.

I therefore propose the switch to database locking. This technique is commonly used across many applications. This pull request implements database locking in a reusable manner. Please check the docstrings and the examples in the unit tests for a better understanding.

[selwin]

Thanks for the PR. Having an optional DB backed lock is a good idea, however, I’d like to make DB based lock optional.

So the file based lock should still be the default (at least for a period of time), but we should allow the user to pass in “lock_type” argument so they can choose which locking mechanism they want to use.

Making locking mechanisms is often tricky, I had to make a few changes to the locking mechanism in the earlier years (even though the initial code is based on other people’s code).

Does this PR reuse code which have been tested somewhere else or is this written from scratch?

[jrief]

@selwin

Having an optional DB backed lock is a good idea, however, I’d like to make DB based lock optional.

OK, that's understandable.

Does this PR reuse code which have been tested somewhere else or is this written from scratch?

In a company I worked for, we used DB locks for all kinds of critical sections. This code was proprietary, written in C++ and worked only against OracleDB. I therefore looked for similar projects for Django. The one which came closest was https://github.com/ambitioninc/django-db-mutex

What I really dislike there is, that a timeout exception is raised after a timed out task finishes. That imo doesn't make any sense at all. Instead I wanted a timeout, which really interrupts a running task, in case it gets stuck. Therefore you have to use processes (instead of threads), because the underlying ALRT-signal is sent to the main thread.
In unit testing I had to use the multiprocessing module in order to emulate concurring tasks.

Unfortunately the unit tests on Travis-CI fail for some reason I haven't understood yet. I will check this and re-add the file locking mechanism.

[jrief]

@selwin I'm still struggling with running unit tests. Reason is the call to connections.close() after threads join. I therefore would like to proceed with a different approach: We should abandon multiprocessing during Celery and Django-Request-Response cycles and only offer it for the command line invocation, ie. ./manage.py send_queued_mail. Multiprocessing inside a running Django instance seems very weird to me, and Celery can handle it much better through its own pool. What do you think?

[selwin]

I was just thinking about this. Instead of a DB based lock, should we implement a memcached/Redis based lock first? Memcached/Redis based locks is significantly easier to implement since they're atomic and can be used on distributed systems too.

[jrief]

I would stay with a database lock. It is the most portable solution, because a database is required anyway and it scales better than file based locks. Adding memcached or Redis to the technology stack adds another moving part, which might be discouraging for first- or rare-time users.

This btw. does not answer my question about abandoning threads and moving multiprocessing into ./manage.py send_queued_mail.

[selwin]

This btw. does not answer my question about abandoning threads and moving multiprocessing into ./manage.py send_queued_mail.

Oh yes, sorry about that.

We should abandon multiprocessing during Celery and Django-Request-Response cycles and only offer it for the command line invocation, ie. ./manage.py send_queued_mail. Multiprocessing inside a running Django instance seems very weird to me, and Celery can handle it much better through its own pool. What do you think?

I don't understand how multiprocessing is used in Django's request/response cycle. The API to send email is mail.send() which either:

1. Saves an email object to DB to be processed in the backgorund
2. If priority is set to now, it will immediately call email.dispatch() which doesn't involve any multiprocessing

So from my perspective, multiprocessing only really comes into play when send_queued_mail is called via CLI.

I'm not familiar with Celery so I'd defer to your judgement. But I'd require that the Celery task that you trigger don't have a process/threads config that's separate from post office (or it should belong to a separate package altogether).

I would stay with a database lock. It is the most portable solution, because a database is required anyway and it scales better than file based locks. Adding memcached or Redis to the technology stack adds another moving part, which might be discouraging for first- or rare-time users.

Ok, in this case I'd suggest also looking into the possibility of using an optional dependency (if there's any) approach.