[Twisted-Python] Inline callbacks: generating the same Deferred at multiple places
Hey everyone, First of all, I am a french developer mostly dealing with security matters, I have been a Twisted enthusiast for a couple of years now and have been hacking around with Twisted for the very last couple of months. I am aware that Deferred-related classes are very stable nowadays and that such a change does not stand half a chance to get anywhere. Anyway, Twisted behavior kind of annoys me when it comes to inline callback management. Indeed, the gotResult function used when generating Deferred objects from a function decorated with inlineCallbacks currently looks like: def gotResult(r): if waiting[0]: waiting[0] = False waiting[1] = r else: _inlineCallbacks(r, g, deferred) So far the function does not return anything explicitely, thus following callbacks get a very nice `None` as first argument. This is not so annoying when using inline callbacks with very simple and straightforward cases as advised by Twisted documentation. Anyway, when generating the same Deferred object from multiple inlineCallbacks-decorated functions, one would expect every `yield` statement to return the same value. As a matter of fact, this is not the case (the first generation of the Deferred object will return its actual current state but later generations will return `None`). Because a Deferred object generation from an inlineCallbacks-decorated function does not allow for anything but waiting for the Deferred object to fire and returning its state, it would probably not harm to forward this state to the next callback as well. Something like: def gotResult(r): if waiting[0]: waiting[0] = False waiting[1] = r else: _inlineCallbacks(r, g, deferred) return r would probably do the trick and allow for complex scenarios where the same Deferred object is generated multiple times by different functions before being fired. As I am not an accomplished Twisted hacker, I cannot say if this kind of change would harm or break any code out there. Yet it seems to me that according to inlineCallbacks behavior, it is very unlikely to have any consequence at all but improving (extending at least) inlineCallbacks behavior. Am I so wrong? (I would very much like someone to point out the one huge detail I completely forgot before I make this change by monkey patching and submit a feature request :) Regards, Pierre. -- Pierre Jaury <pierre@jaury.eu> Weblog - http://kaiyou.fr GPG ID - E804FB60
On Fri, Feb 22, 2013 at 9:40 AM, Pierre Jaury <pierre@jaury.eu> wrote:
would probably do the trick and allow for complex scenarios where the
I think it's a reasonable change to make, and I don't foresee any problems with it, so I think it's fine to submit a bug about it. But I do question the architecture that needs to make use of it. I would probably avoid scenarios like that in my own code. -- Christopher Armstrong http://radix.twistedmatrix.com/ http://planet-if.com/
On Feb 22, 2013, at 8:30 AM, Christopher Armstrong <radix@twistedmatrix.com> wrote:
I think it's a reasonable change to make, and I don't foresee any problems with it, so I think it's fine to submit a bug about it. But I do question the architecture that needs to make use of it. I would probably avoid scenarios like that in my own code.
I disagree; the behavior of result consumption is intentional - although it could be better documented. Changing it would very definitely be incompatible (<http://twistedmatrix.com/trac/wiki/CompatibilityPolicy>); this is possible, of course, if the deprecation/migration is worth it, but the behavior being requested here would be worse in a number of ways. If we re-populated the result, every failed Deferred yielded by an inlineCallbacks function would log its traceback twice: once when the unhandled exception propagated out of the inlineCallbacks function causing its Deferred to fail, and once when the unhandled exception propagated from the yielded Deferred itself, since nothing would have consumed it when that Deferred would be GC'd. Speaking of GC, similarly, any large objects in Deferred results processed by inlineCallbacks functions would live longer, and continue participating in any reference cycles they're part of, possibly causing memory leaks, or at least, longer collection times and less favorable memory usage behavior, especially in long-lived processes. Basically, you can't treat a Deferred as an event broadcaster. It isn't one. It's a single-shot representation of an asynchronous result, which a single consumer can consume with its current value, possibly yielding a new value. Some consumers can be diligent about not modifying the Deferred's state, so that it can be passed on down the chain, but inlineCallbacks can never be such a consumer: since each inlineCallbacks-decorated function generates its own Deferred return value, it is naturally the terminal consumer of any Deferreds that it yields, and should clear out their results. Ultimately, _every_ Deferred ought to have a terminal consumer, that takes the result and does useful work with it - persists it, shows it in some UI to a user - rather than continuing to pass it along. Since 'yield x' is not sufficiently expressive to say what else to do with 'x' and what state to leave it in, we must assume that the intention of a coroutine is to take the value and do some work with it. Any inlineCallbacks function which wants to express its intent more precisely can do this, instead: def fork(d): d2 = Deferred() def fire(x): d2.callback(x) return x d.addBoth(fire) return d2 @inlineCallbacks def foo(): result = yield fork(somethingAsync()) Maybe putting that function in Twisted (this is not the first time it's come up) would be a useful addition. -glyph
On 02/22/2013 09:17 PM, Glyph wrote:
Well, some criticism, now I am listening to you.
Well, this simply means that the change is not that simple to make and would probably imply some deeper modifications. I can see how this kills any hope to get the change merged into anything stable anytime soon however.
But.. I definitely agree with this one, which I did not foresee. I therefore agree that any change in Deferred objects behavior should never enforce references to live longer when not usually required.
That I figured out already. Thanks for the reminder anyway, I was on the path of loosing myself, there :)
I will embed this one in my bundle of Twisted utility functions. Thanks!
Maybe putting that function in Twisted (this is not the first time it's come up) would be a useful addition.
Agreed, then. -- Pierre Jaury <pierre@jaury.eu> Weblog - http://kaiyou.fr GPG ID - E804FB60
On Feb 23, 2013, at 3:23 AM, Pierre Jaury <pierre@jaury.eu> wrote:
The change cannot be made, as such. The compatibility policy page, linked above, details what would be necessary to add a new API with the behavior you want, but as I think I made clear already, I think that would be a bad idea ;).
Deferred holds enough references to get its job done, no more, no less. (Well, maybe a little more in the case of a failed Deferred. But mostly we are talking about a successful Deferred here.)
OK, good :).
Some existing implementations: <http://trac.calendarserver.org/browser/CalendarServer/trunk/twext/enterprise...> <http://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Epsilon/...> -glyph
On Fri, Feb 22, 2013 at 9:40 AM, Pierre Jaury <pierre@jaury.eu> wrote:
would probably do the trick and allow for complex scenarios where the
I think it's a reasonable change to make, and I don't foresee any problems with it, so I think it's fine to submit a bug about it. But I do question the architecture that needs to make use of it. I would probably avoid scenarios like that in my own code. -- Christopher Armstrong http://radix.twistedmatrix.com/ http://planet-if.com/
On Feb 22, 2013, at 8:30 AM, Christopher Armstrong <radix@twistedmatrix.com> wrote:
I think it's a reasonable change to make, and I don't foresee any problems with it, so I think it's fine to submit a bug about it. But I do question the architecture that needs to make use of it. I would probably avoid scenarios like that in my own code.
I disagree; the behavior of result consumption is intentional - although it could be better documented. Changing it would very definitely be incompatible (<http://twistedmatrix.com/trac/wiki/CompatibilityPolicy>); this is possible, of course, if the deprecation/migration is worth it, but the behavior being requested here would be worse in a number of ways. If we re-populated the result, every failed Deferred yielded by an inlineCallbacks function would log its traceback twice: once when the unhandled exception propagated out of the inlineCallbacks function causing its Deferred to fail, and once when the unhandled exception propagated from the yielded Deferred itself, since nothing would have consumed it when that Deferred would be GC'd. Speaking of GC, similarly, any large objects in Deferred results processed by inlineCallbacks functions would live longer, and continue participating in any reference cycles they're part of, possibly causing memory leaks, or at least, longer collection times and less favorable memory usage behavior, especially in long-lived processes. Basically, you can't treat a Deferred as an event broadcaster. It isn't one. It's a single-shot representation of an asynchronous result, which a single consumer can consume with its current value, possibly yielding a new value. Some consumers can be diligent about not modifying the Deferred's state, so that it can be passed on down the chain, but inlineCallbacks can never be such a consumer: since each inlineCallbacks-decorated function generates its own Deferred return value, it is naturally the terminal consumer of any Deferreds that it yields, and should clear out their results. Ultimately, _every_ Deferred ought to have a terminal consumer, that takes the result and does useful work with it - persists it, shows it in some UI to a user - rather than continuing to pass it along. Since 'yield x' is not sufficiently expressive to say what else to do with 'x' and what state to leave it in, we must assume that the intention of a coroutine is to take the value and do some work with it. Any inlineCallbacks function which wants to express its intent more precisely can do this, instead: def fork(d): d2 = Deferred() def fire(x): d2.callback(x) return x d.addBoth(fire) return d2 @inlineCallbacks def foo(): result = yield fork(somethingAsync()) Maybe putting that function in Twisted (this is not the first time it's come up) would be a useful addition. -glyph
On 02/22/2013 09:17 PM, Glyph wrote:
Well, some criticism, now I am listening to you.
Well, this simply means that the change is not that simple to make and would probably imply some deeper modifications. I can see how this kills any hope to get the change merged into anything stable anytime soon however.
But.. I definitely agree with this one, which I did not foresee. I therefore agree that any change in Deferred objects behavior should never enforce references to live longer when not usually required.
That I figured out already. Thanks for the reminder anyway, I was on the path of loosing myself, there :)
I will embed this one in my bundle of Twisted utility functions. Thanks!
Maybe putting that function in Twisted (this is not the first time it's come up) would be a useful addition.
Agreed, then. -- Pierre Jaury <pierre@jaury.eu> Weblog - http://kaiyou.fr GPG ID - E804FB60
On Feb 23, 2013, at 3:23 AM, Pierre Jaury <pierre@jaury.eu> wrote:
The change cannot be made, as such. The compatibility policy page, linked above, details what would be necessary to add a new API with the behavior you want, but as I think I made clear already, I think that would be a bad idea ;).
Deferred holds enough references to get its job done, no more, no less. (Well, maybe a little more in the case of a failed Deferred. But mostly we are talking about a successful Deferred here.)
OK, good :).
Some existing implementations: <http://trac.calendarserver.org/browser/CalendarServer/trunk/twext/enterprise...> <http://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Epsilon/...> -glyph
participants (3)
-
Christopher Armstrong -
Glyph -
Pierre Jaury