Allow deferral of ModelSignal callback invocation until after transaction commit

[Christopher Adams]

So wrote an RFC and a prototype branch in Django to defer the invocation of certain handlers registered with Django model signals optionally until after transaction commit. Any thoughts?

[Carl Meyer]

Hi,

On 04/27/2015 08:17 AM, Christopher Adams wrote:
> So wrote an
> <https://docs.google.com/document/d/1VtSduRYtAIZa4sOFvj_H0f3JfgXxf_vbxQ4RTlFBags/edit?usp=sharing>RFC
> <https://docs.google.com/document/d/1VtSduRYtAIZa4sOFvj_H0f3JfgXxf_vbxQ4RTlFBags/edit?usp=sharing> and a
> prototype
> <https://github.com/django/django/compare/django:master...adamsc64:modelsignal_sent_after_commit?expand=1>

> branch in Django to defer the invocation of certain handlers registered
> with Django model signals optionally until after transaction commit. Any
> thoughts?

Thanks for putting the work into writing up this clear proposal, with
prototype code!

I agree that it would be good to have the ability in Django to run code
after-commit, but I think that feature is more useful it it's not
coupled to the signals framework. It would be better to have a generic
way to say "run this code after the current transaction commits"; then
it's equally easy to make use of that hook in a model signal handler or
elsewhere.

I wrote django-transaction-hooks [1] to serve this use case. I think
probably something like django-transaction-hooks should be part of
Django core. I'd be interested in any feedback you have on it.

Carl

[1] https://django-transaction-hooks.readthedocs.org/en/latest/

[Aymeric Augustin]

On 27 avr. 2015, at 16:17, Christopher Adams <christoph...@gmail.com> wrote:

So wrote an RFC and a prototype branch in Django to defer the invocation of certain handlers registered with Django model signals optionally until after transaction commit. Any thoughts?

Hi Christopher,

Thanks for working on this. In addition to what Carl said, have you read https://code.djangoproject.com/ticket/21803 and https://code.djangoproject.com/ticket/14051? I think they’re relevant here.

-- 

Aymeric.

[Andrew Godwin]

I agree that perhaps making it part of signals is not in everyone's favour (as signals are kind of unloved) but I also like the simplicity of Christopher's approach - the patch is small and understandable, and using it is pretty easy (especially to upgrade existing code into "safe" code).

Unless it's going to be super easy to port transaction-hooks over, I think it might be nice to get this in and sealed (it's not adding too much more complexity). If transaction-hooks would land in core well, let's do that.

As for the two bugs, I think it would solve the underlying problem, but not the bug as described (e.g. for 14051, it would stop refs being pushed in for uncommitted transactions). Still fine IMO, and definitely leave comments on them noting this if it does land.

Andrew

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CC456857-820D-45EB-A515-F62007041ABB%40polytechnique.org.

For more options, visit https://groups.google.com/d/optout.

[Carl Meyer]

Hi Andrew,

On 04/30/2015 06:13 AM, Andrew Godwin wrote:
> I agree that perhaps making it part of signals is not in everyone's
> favour (as signals are kind of unloved) but I also like the simplicity
> of Christopher's approach - the patch is small and understandable, and
> using it is pretty easy (especially to upgrade existing code into "safe"
> code).

transaction-hooks is actually fairly small and understandable too. And I
don't think it's hard to use for this situation, either; you'd just need
to use `connection.on_commit` in your signal handler if you wanted to
delay some action until post-commit.

> Unless it's going to be super easy to port transaction-hooks over, I
> think it might be nice to get this in and sealed (it's not adding too
> much more complexity). If transaction-hooks would land in core well,
> let's do that.

I don't think landing transaction-hooks in core for 1.9 would be hard at
all, and no-one has objected to the idea (AFAIK). I (or anyone really)
just need to get around to it. Motivation has been low so far mostly
because it works fine as an external add-on.

Overall, I'm -1 on merging the signals patch. I think ending up with two
different ways to do the same thing (one of which is specific to
signals) is a net negative, and I think the `connection.on_commit()`
approach is clearer and more flexible.

Carl

[Andreas Pelme]

Hi,

> On 30 apr 2015, at 18:42, Carl Meyer <ca...@oddbird.net> wrote:
>
> transaction-hooks is actually fairly small and understandable too. And I
> don't think it's hard to use for this situation, either; you'd just need
> to use `connection.on_commit` in your signal handler if you wanted to
> delay some action until post-commit.
>
>> Unless it's going to be super easy to port transaction-hooks over, I
>> think it might be nice to get this in and sealed (it's not adding too
>> much more complexity). If transaction-hooks would land in core well,
>> let's do that.
>
> I don't think landing transaction-hooks in core for 1.9 would be hard at
> all, and no-one has objected to the idea (AFAIK). I (or anyone really)
> just need to get around to it. Motivation has been low so far mostly
> because it works fine as an external add-on.

I did an initial port of django-transaction-hooks, it was pretty straightforward. All the hard work has already been done. :-)

Here is the PR: https://github.com/django/django/pull/4593

The patch is not yet finished (there’s a todo-list at the PR with some missing pieces). Let me know what you think and I’ll be happy to continue working on a proper patch to get it into a merge-able state.

Cheers,
Andreas

[Christopher Adams]

Hey guys,

Thanks for the great feedback and replies.

Generally agree with everyone that post-commit hooks shouldn't be strictly coupled to the signals framework philosophically speaking.

I disagree with Carl's premise that using `connection.on_commit` in your signal handler is simpler than adding `after_commit=True` as a kwarg. It involves not only an extra import, but also familiarity by developers with the notion of closure and callbacks. Despite being a powerful feature of Python, this is not really as familiar a usage pattern among developers as adding an additional keyword argument is. I think that having the option to add `after_commit=True` to a ModelSignal registration point will be a popular addition to Django. I am speaking from the simplicity of the Python-API perspective we expose here. And, from experience -- at Venmo we see this problem every day and would love a simple binary-flag solution like that. I'm assuming many people have ModelSignals that they would love to upgrade with a single change like that.

I'm a little confused by what Carl is saying that my branch would require two ways of doing the same thing. Carl I don't think this is true; I could easily rename my function `add_callback_after_commit` to `on_commit` and extend it. As far as I can see, there are more detailed semantics around manual commits as well as savepoint commits that your project implements that I don't have in my branch. Then again, I don't claim to. I don't see the branches as incompatible. The implementation for the signal handler syntax I came up with can very easily be upgraded to register its own callback to `on_commit()` underneath the hood, when that is merged. In other words, I think these branches could work together and don't need to be seen as alternatives.

The advantages of merging my branch now are: 1. it's stable, and it works; 2. it's a small patch, 3. it guarantees we get a fix to the `post_save` usage problem into next version of Django, period, regardless of the progress of Andreas' branch, 4. It doesn't prevent us from connecting it to whatever final syntax we agree on to going forward (Carl's `on_commit()`, or whatever), and 5. (now personally speaking), I'd like at least to get some credit for driving the idea here, and to get some of this into Django commit history ;). I don't need to take the whole pie but it would be great to get my commits in *if* the agreement is the kwarg syntax is useful at all.

Happy to work with everyone to make more generic post-commit hook happen in addition to what I have already done.

Let me know if I have clearance to open a PR? I don't want to jump the gun there and open without formal approval. Thanks.

Chris

[Shai Berger]

Hi,

On Saturday 02 May 2015 07:20:00 Christopher Adams wrote:
> Hey guys,
>
> Thanks for the great feedback and replies.
>
> Generally agree with everyone that post-commit hooks shouldn't be strictly
> coupled to the signals framework philosophically speaking.
>
> I disagree with Carl's premise that using `connection.on_commit` in your
> signal handler is simpler than adding `after_commit=True` as a kwarg.

I think you're confusing "simple" with "easy", see
http://www.infoq.com/presentations/Simple-Made-Easy

In particular, you are proposing to make one use-case easier, while making the
semantics of the API much more complex (the most glaring point: With your
proposal, adding a keyword argument changes the signal handler invocation from
synchronous to asynchronous, with everything that entails about error
handling). I am -1 on that API, regardless of the "one way to do it" argument.

> [using `connection.on_commit`] involves not only an extra import, but also

> familiarity by developers with the notion of closure and callbacks. Despite
> being a powerful feature of Python, this is not really as familiar a usage
> pattern among developers

It is quite easy to hide the use of connection.on_commit, including the
concepts of callback and closure, behind a small wrapper for handler
registration. I'd hesitate before putting it in core, but you can prove me
wrong by releasing it as a utility (or even a Django Snippet) and making it
popular.

HTH,
Shai.

[Christopher Adams]

Shai,

Thanks for the note.

I thought about what you wrote a bit. I find your points compelling. I don't like the consequences of my proposed changes in terms of:

1. Error handling. The exceptions would be raised at the end of the transaction block and there's no simple way to handle them except to wrap the whole block in another large try-except block, or have some means of registering an on_error callback (which I'd be pretty much against introducing into signals).

2. The semantics around the respective behaviors of `send` and `send_robust` get more complicated, as exit state of the handlers couldn't be ensured to be returned in the tuples synchronously. I think this is also what you are implying by this point. This is a big change in the API guarantees there.

3. The semantics of `on_commit` have similar complications around raising exceptions. But that may be worth solving, and I think we might as well have new API semantics for that that aren't mixed into signals API.

So unless anyone has objections, I'm going to put my branch on hold for now. If anyone still wants me to see if there's a way it can work I'm willing to give it a bit more work, however I think it should probably be passed over for the time being. Glad my post may have kicked off the conversation to get a eventual solution in.

Next steps: is everyone agreed that attempting to merge Carl's django-transaction-hooks is the way forward? Happy to help on that if you guys need help. Pretty excited to get a post-commit hook into core in 1.9, no matter how that ends up happening.

Best,

Chris

[Andreas Pelme]

> On 3 maj 2015, at 15:44, Christopher Adams <christoph...@gmail.com> wrote:
>
> So unless anyone has objections, I'm going to put my branch on hold for now. If anyone still wants me to see if there's a way it can work I'm willing to give it a bit more work, however I think it should probably be passed over for the time being. Glad my post may have kicked off the conversation to get a eventual solution in.
>
> Next steps: is everyone agreed that attempting to merge Carl's django-transaction-hooks is the way forward? Happy to help on that if you guys need help. Pretty excited to get a post-commit hook into core in 1.9, no matter how that ends up happening.

Christopher: Thank you very much for your efforts on this, I am also very excited to get some post-commit hook into Django. This has been a long standing issue for me personally, having run into this in different projects in different situations over the years. Lately I’ve been happily using django-transaction-hooks, but at the same time been sad that it wasn’t part of Django. You’re post made me jump in and try to port django-transaction-hooks.

There is some work/discussions/progress going on in the PR at https://github.com/django/django/pull/4593. Currently most things seems to work and the docs is written. The most tricky issue (related to sqlite autocommit semantics) has hopefully been solved.

Any review of the code/tests/docs and input on the open questions would be very welcome and helpful!

Cheers,
Andreas