On Jul 28, 2014, at 1:54 AM, Laurens Van Houtven <_@lvh.io> wrote:

Honored twistedeers,

Seriously, we need to come up with a good collective noun for ourselves one day.  We should have a referendum or something.

Consider the following (blocking) decorator, which runs a function in a transaction:

def _with_transaction(f):
    def decorated(self, *args, **kwargs):
...
       try:
            result = f(self, conn, *args, **kwargs)
        except:
            txn.rollback()
            raise
...
Where I to translate this logic verbatim to @inlineCallbacks, I get:

def _with_transaction(f):
    @inlineCallbacks
    def decorated(self, *args, **kwargs):
...
        try:
            result = yield f(self, conn, *args, **kwargs)
        except:
            yield txn.rollback()
            raise
...

However, there’s a bug here! In the except clause: there’s an (implicit) current exception, to be re-raised by the bare raise statement. Unfortunately, when doing yield txn.rollback(), that conveniently eats said exception.

This is actually true of _with_transaction as well.  Any code, anywhere, might call sys.exc_clear(), or do some interpreter shenanigans that accidentally make the equivalent happen.  So when you are calling "rollback" on your transaction and perhaps running application code in a post-rollback hook (because your database has that, right?  you totally need it for some things).

And of course rollback can *itself* fail which wipes out the exception anyway - is that what you want to happen in that condition?

Of course, there’s a fairly simple workaround involving catching BaseException and capturing the exception instance explicitly.

I’m wondering if this is just a leaky abstraction, or if I should report it as a bug in @inlineCallbacks?

I don't think there's any such thing as "just" a leaky abstraction :-).  If an abstraction leaks in an unspecified way, it's a bug.  Report away.

Personally, I'd do something like this:

try:
    ...
except:
    captured = Failure()
    yield txn.rollback()
    yield captured

because I find that it's more readable - rather than relying on ephemeral, manipulable state which should (but doesn't necessarily) follow conventions related to indentation, I explicitly capture the implicit state, on the only line of code where it's guaranteed to be the right thing.

Also worth noting that yielding the Failure will fail the Deferred returned by your inlineCallbacks function, implicitly terminating the execution of that function.

-glyph