Hook Methods are called while the transaction is being executed not when the transaction is completed successfully

[GoogleCodeExporter]

the hook methods are being called while executing transactions instead of when 
the transaction is successful and complete.

i am using the _post_put_hook for some business logic.
while in a transaction the _post_put_hook get called when the model object put 
is called not when the transaction is complete.

so, if the transaction is retried then i get multiple _post_put_hook which is 
bad.  also, the transaction may fail but the _post_put_hook was already called 
even know the true db did not get updated. 

The hooks should only be called at the end of a successful transaction.   

Original issue reported on code.google.com by fr...@twist.com on 7 Apr 2012 at 4:33

[GoogleCodeExporter]

Hm... I think you have a point, but it's not easy to change the semantics 
without breaking backward compatibility.

I wonder if you could implement some wrapper around transaction() that 
implements a queue of callbacks. Then in your _post_put_hook you can do 
something like

class ...(ndb.Model):
  ...
  def _post_put_hook(self, fut):
    if ndb.in_transaction():
      queue.add(self.real_post_hook)
    else:
      self.real_post_hook()
  def real_post_hook(self):
    <whatever you wish>

Then the transaction wrapper could throw away the queue when the transaction 
fails or execute the callbacks in the queue when it succeeds.

It's a little tricky to find a good place for the queue; making it a global 
would obviously be bad, and even making it thread-safe would be bad in case you 
have multiple concurrent (async) transactions. Storing it on the context would 
be tricky too, because inside the transaction a different context is used, but 
the wrapper does not have access to that (it is created inside 
Context.transaction() and never exported from it).

So, in conclusion we would probably have to implement such a queue as a feature 
of the Context class. The feature could even include the code I put in the body 
of _post_put_hook() above. BUt it would still require doing something special 
in _post_put_hook() if you want this behavior. (Possibly we could use a 
decorator for that?)

What do you think?

Original comment by guido@google.com on 7 Apr 2012 at 9:44

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Here's a tentative implementation (needs unittests, has 1-2 unrelated fixes):
http://codereview.appspot.com/5988060

Original comment by guido@google.com on 7 Apr 2012 at 11:15

[GoogleCodeExporter]

We're having a little API design discussion about this internally. Does anybody 
have a use case for "rollback hooks", which  would be called when a commit is 
*not* successful? I can only think of uses for this that I would consider 
undesirable, such as updating memcache and reverting the update on rollback...

And there's the caveat that it's always possible that a transaction is rolled 
back but the process crashes before the callback runs. (Of course, that's also 
possible for the success case.)

Original comment by guido@google.com on 26 Apr 2012 at 7:36

[GoogleCodeExporter]

Rollback use case: maybe launching audit or user notification tasks? I can't 
think of anything specific, but having a commit hook certainly begs for a 
rollback hook.

Re: "process crashes before the callback runs": we need to deal with that in 
almost everything we do, no? I.e., we're always just operating over a set of 
9's in terms of likelihood of our code executing. This doesn't seem any 
different than that.

That said, in the tentative patch, I noticed your "this better not raise" 
comment. I guess you'd have to just let the exception bubble out?

Original comment by jason.a....@gmail.com on 26 Apr 2012 at 10:07

[GoogleCodeExporter]

I'll call YAGNI on the rollback callback until someone comes up with a real use 
case...

And indeed hard crashes are always a possibility... Did you know it's even 
possible that the client sees the transaction *fail* but (due to a severe 
network partition) the datastore actually makes it commit?

Re: "this better not raise" -- it's the same attitude we have elsewhere -- if a 
callback raises the error will bubble up and the remaining callbacks won't run.

Original comment by guido@google.com on 26 Apr 2012 at 10:12

• Changed state: Started

[GoogleCodeExporter]

This issue was closed by revision 82112ad49076.

Original comment by guido@google.com on 28 Apr 2012 at 1:08

• Changed state: Fixed