django-celery-transactions icon indicating copy to clipboard operation
django-celery-transactions copied to clipboard

Django 1.9 compatibility

Open akuchling opened this issue 7 years ago • 3 comments

I work on a project that uses django-celery-transactions and Django 1.8, but would like to upgrade to Django 1.9. The README warns the 1.9 "will likely require refactoring", so I investigated it a bit and the result is better than expected.

It looks like django-celery-transactions is fine with Django 1.9, despite 1.9's addition of an on_commit() hook, and the monkey-patching of Atomic.__exit__ will not disable any existing features. Django's on_commit() is actually a method on database connections and doesn't require any code in transaction.Atomic's __exit__ method. That __exit__ method seems to be identical between 1.8 and 1.9, except for a 2-line change; Django commit 00a1d4d042a7afd139316982c9b57e87d26a894f removes the 'elif connection.features.autocommits_when_autocommit_is_off' block.

Unfortunately it looks like we can't just drop the monkey-patching and use 1.9's on_commit feature because it's not sufficient to implement what djcelery-transactions needs. 1.9 only has an on-commit hook, and the indexing needs a rollback hook because it needs to either send the tasks (in the case of a successful commit) or discard them (in the case of a rollback). Maybe we could add a wrapper for the __enter__ method that cleared the task queue on initial entry to an atomic block?

I can file a pull request to update the README if you like.

akuchling avatar Apr 06 '17 14:04 akuchling

I think it runs on Django 1.9 but I do not think it runs as expected.

On different project I ended-up dropping django-celery-transactions and djcelery as recommended by the celery project. Instead I started calling tasks with the on_commit() syntax.

Thanks for the Pull Request on the README. We can merge, but I'm not sure it's worth maintaining the changes for 1.9+ support at this point.

nicolasgrasset avatar Apr 07 '17 19:04 nicolasgrasset

You write "I do not think it runs as expected." Can you please expand on what you expect to not work? From examining the code and from trying it using celery-simple-elasticsearch, things do in fact seem to work fine. The code: with transaction.atomic(): obj.save() records the task for updating the object's index, and the task is sent on exiting the atomic block.

If I put 1/0 into the atomic block, the exception triggers a rollback and no task is sent.

(This is of course risky if Django versions after 1.9 make further changes to Atomic.exit, but that's not what I'm concerned about right now.)

akuchling avatar Apr 10 '17 18:04 akuchling

@fellowshipofone:

Regarding calling tasks with on_commit syntax. Do you mean something like this?

Transaction aware task

class TransactionAwareTask(Task):
    '''
    Task class which is aware of django db transactions and only executes tasks
    after transaction has been committed
    '''
    abstract = True

    def apply_async(self, *args, **kwargs):
        '''
        Unlike the default task in celery, this task does not return an async
        result
        '''
        transaction.on_commit(
            lambda: super(TransactionAwareTask, self).apply_async(
                *args, **kwargs))

Usage of the transaction aware task

@shared_task(base=TransactionAwareTask, bind=True, max_retries=settings.EVENT_MAX_RETRIES)
def push_event(self, event_id):
    event = Event.objects.get(id=event_id)
    # code to push webhook

barseghyanartur avatar May 31 '17 14:05 barseghyanartur