[Twisted-Python] inlineCallbacks and current exceptions: leaky abstraction or bug?
Honored twistedeers, Consider the following (blocking) decorator, which runs a function in a transaction: def _with_transaction(f): def decorated(self, *args, **kwargs): conn = self.engine.connect() txn = conn.begin() try: result = f(self, conn, *args, **kwargs) except: txn.rollback() raise else: txn.commit() return return decorated Where I to translate this logic verbatim to @inlineCallbacks, I get: def _with_transaction(f): @inlineCallbacks def decorated(self, *args, **kwargs): conn = yield self.engine.connect() txn = yield conn.begin() try: result = yield f(self, conn, *args, **kwargs) except: yield txn.rollback() raise else: yield txn.commit() returnValue(result) return decorated 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. 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? cheers lvh
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
On 07/28/2014 06:56 PM, Glyph Lefkowitz wrote:
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.
I vote for "minions" ... :) Steve
Bare except and bare raise are both bad Python style in general, for exactly the reasons that lvh and glyph have identified. That inlineCallbacks (might?) turn "bad style as it might fail when you least expect it" into "always fails" doesn't seem like a bug to me -- in fact, by removing some nondeterminism[1] I think it's a feature! Dustin [1] Of the worst kind: based on deployment details, library versions, configuration, and DB server behavior, rather than truly random On Mon, Jul 28, 2014 at 7:44 PM, Steve Waterbury <waterbug@pangalactic.us> wrote:
On 07/28/2014 06:56 PM, Glyph Lefkowitz wrote:
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.
I vote for "minions" ... :)
Steve
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On 29 Jul 2014, at 6:56, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
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.
I’ve used the term “twistedgeers” before (rhymes with engineers), which sounds much nicer said than when it is typed. -hawkowl
participants (5)
-
Dustin J. Mitchell -
Glyph Lefkowitz -
HawkOwl -
Laurens Van Houtven -
Steve Waterbury