[Twisted-Python] inlineCallbacks and current exceptions: leaky abstraction or bug?
![](https://secure.gravatar.com/avatar/1846c8040fcf70e9b55bb7bfcdb78bc4.jpg?s=120&d=mm&r=g)
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
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
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.
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
![](https://secure.gravatar.com/avatar/96342dbb350e3be883a4cb2f7c3f2423.jpg?s=120&d=mm&r=g)
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:
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
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.
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
![](https://secure.gravatar.com/avatar/96342dbb350e3be883a4cb2f7c3f2423.jpg?s=120&d=mm&r=g)
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:
participants (5)
-
Dustin J. Mitchell
-
Glyph Lefkowitz
-
HawkOwl
-
Laurens Van Houtven
-
Steve Waterbury