[Twisted-Python] why can't a callback be called with a deferred?
Hi All, There's this assert: https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.py#... ...and I'd like to understand why it's there. I have what feels like a legitimate case where that assert trips me up, as I'd like to change this line: https://github.com/cjw296/carly/blob/d39e316aa0f9e05613c263410e7b3ba5bcacba3... ...to just return result.value. However, if the method being hooked returns a deferred, then I trip the assert. Unfortunately, the commit that introduced that assert is just a one liner from Itamar back in 2003 without any context of what problem lead him to introduce the change. Of course, I can work around it, was more curious than anything... Chris
On Tue, 19 Feb 2019 at 11:01, Chris Withers <chris@withers.org> wrote:
Hi All,
There's this assert:
https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.py#...
...and I'd like to understand why it's there.
[snip]
Unfortunately, the commit that introduced that assert is just a one liner from Itamar back in 2003 without any context of what problem lead him to introduce the change.
I think it was introduced to catch some common bad usage patterns ... like yours :) If you want to chain the deferreds, use the dedicated helper https://twistedmatrix.com/documents/current/core/howto/defer.html#chaining-d... Deferred are not always 100% resolved/called. You might have a deferred called, but the current result might be another deferred... so it has no final result yet. ---- so in your case, instead of `returnValue(result)` use result = yield result returnValue(result) in this way, the result is resolved :) Hope it helps -- Adi Roiban
On 19/02/2019 11:41, Adi Roiban wrote:
I think it was introduced to catch some common bad usage patterns ... like yours :)
Not a massively helpful comment.
If you want to chain the deferreds, use the dedicated helper https://twistedmatrix.com/documents/current/core/howto/defer.html#chaining-d...
Deferred are not always 100% resolved/called. You might have a deferred called, but the current result might be another deferred... so it has no final result yet.
----
so in your case, instead of `returnValue(result)` use
result = yield result returnValue(result)
in this way, the result is resolved :)
The methods being hooked don't necessarily return deferreds. I'd like it to be an explicit choice of the caller, ie: result = yield SomeProtocol.onMessage.called() # okay, we got here, we know onMessage was called, # now we might want to tick a clock, or otherwise simulate # async state manipulation. # now I want to make sure the deferred chain on the onMessage result has been completed: yield result cheers, Chris
On Feb 19, 2019, at 5:34 AM, Chris Withers <chris@withers.org> wrote:
On 19/02/2019 11:41, Adi Roiban wrote:
I think it was introduced to catch some common bad usage patterns ... like yours :)
Not a massively helpful comment.
I think what Adi was trying to get at here is that this is exactly the sort of type confusion that this assert was intended to defend against. That said, the bare assert in question was definitely written with the implementor's sort of "this should never happen" hat on, not the "let's report a useful error to the user" hat. So it could certainly stand to be improved regardless.
If you want to chain the deferreds, use the dedicated helper https://twistedmatrix.com/documents/current/core/howto/defer.html#chaining-d... <https://twistedmatrix.com/documents/current/core/howto/defer.html#chaining-d...>
Deferred are not always 100% resolved/called. You might have a deferred called, but the current result might be another deferred... so it has no final result yet.
----
so in your case, instead of `returnValue(result)` use
result = yield result returnValue(result)
in this way, the result is resolved :)
The methods being hooked don't necessarily return deferreds. I'd like it to be an explicit choice of the caller, ie:
result = yield SomeProtocol.onMessage.called() # okay, we got here, we know onMessage was called, # now we might want to tick a clock, or otherwise simulate # async state manipulation. # now I want to make sure the deferred chain on the onMessage result has been completed: yield result
I'm not sure I understand your example here. The assertion in question only happens if you call returnValue or do a return with a Deferred directly; this example doesn't do either of those things. I think you could fix the linked example code to still do what you want by simply doing `returnValue(yield result)`? -g
On 21/02/2019 06:55, Glyph wrote:
The methods being hooked don't necessarily return deferreds.
Glyph, this bit ^^^
I'd like it to be an explicit choice of the caller, ie:
result = yield SomeProtocol.onMessage.called() # okay, we got here, we know onMessage was called, # now we might want to tick a clock, or otherwise simulate # async state manipulation. # now I want to make sure the deferred chain on the onMessage result has been completed: yield result
I'm not sure I understand your example here.
Yeah, this is part of carly, that I posted earlier. It stems from the need to get the results of method calls when you have no reference to the object being calls, or sometimes a result that's a deferred you need to wait on, particularly in a test, but have no way of doing so. If you're feeling brave, have a read of: https://github.com/cjw296/carly/blob/master/carly/hook.py
The assertion in question only happens if you call returnValue or do a return with a Deferred directly; this example doesn't do either of those things.
This is the test situation where I hit this issue: https://github.com/cjw296/carly/blob/master/tests/test_untracked_deferred.py... I'd originally wanted to have that read: @inlineCallbacks def test1(self): ... result = yield pita.asyncMethod.called() with ShouldRaise(Exception(1)): yield result Now, which I'm actually happier with the end result here, I think the above it legit, if unusual, and that assert trips it up. cheers, Chris
On Tuesday, 19 February 2019 11:00:57 GMT Chris Withers wrote:
Hi All,
There's this assert:
https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.py# L459
...and I'd like to understand why it's there.
We hit this assert when porting from very old twisted to current twisted. In all cases the problem was with our code that used deferreds in a poor, not well understood way. After refactoring we are a lot happier with the resulting code as it easier to maintain now. Barry
On Feb 25, 2019, at 3:32 AM, Scott, Barry <barry.scott@forcepoint.com> wrote:
On Tuesday, 19 February 2019 11:00:57 GMT Chris Withers wrote:
Hi All,
There's this assert:
https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.py# L459
...and I'd like to understand why it's there.
We hit this assert when porting from very old twisted to current twisted. In all cases the problem was with our code that used deferreds in a poor, not well understood way. After refactoring we are a lot happier with the resulting code as it easier to maintain now.
Thanks for the feedback, Barry! It would still be great to figure out, if we can, how we might make the error message a bit more legible to folks with less knowledge of Twisted's internals. -g
On Tuesday, 26 February 2019 06:34:28 GMT Glyph wrote:
On Feb 25, 2019, at 3:32 AM, Scott, Barry <barry.scott@forcepoint.com> wrote:> On Tuesday, 19 February 2019 11:00:57 GMT Chris Withers wrote:
Hi All,
There's this assert:
https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer. py# L459
...and I'd like to understand why it's there.
We hit this assert when porting from very old twisted to current twisted. In all cases the problem was with our code that used deferreds in a poor, not well understood way. After refactoring we are a lot happier with the resulting code as it easier to maintain now.
Thanks for the feedback, Barry!
It would still be great to figure out, if we can, how we might make the error message a bit more legible to folks with less knowledge of Twisted's internals.
Let suppose that I need work done by doWork function. It returns a deferred for me to hang call backs and error backs on. d = doWork() d.addCallback(handleWorkDone) In my handleWorkDone I expect to get the result of doWork completing. The assert fires if instead of a result value is returned a Deferred is returned. This I consider a bug in the doWork() implementation. What must happen in doWork is that it must arrange that any Deferred it used internally has an addCallback used to cause the d returned to the user to complete. Leaking the any internal Deferred() objects must not happen to the user of doWork. def doWork(): d = Deferred() def completeWork(result, d): d.callback(result) inner_d = doAsyncWork() inner_d.addCallback(completeWork, d) return d The error message would need to say something like: "Cannot return a Deferred as a result. Did you forgot to addCallback to the deferred?" Maybe add something to docs based on the above and refer to it in the message? Barry
On Wed, Feb 27, 2019 at 9:34 AM Scott, Barry <barry.scott@forcepoint.com> wrote:
On Feb 25, 2019, at 3:32 AM, Scott, Barry <barry.scott@forcepoint.com> wrote:> On Tuesday, 19 February 2019 11:00:57 GMT Chris Withers wrote:
Hi All,
There's this assert:
https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.
py# L459
...and I'd like to understand why it's there.
We hit this assert when porting from very old twisted to current twisted. In all cases the problem was with our code that used deferreds in a
not well understood way. After refactoring we are a lot happier with
On Tuesday, 26 February 2019 06:34:28 GMT Glyph wrote: poor, the
resulting code as it easier to maintain now.
Thanks for the feedback, Barry!
It would still be great to figure out, if we can, how we might make the error message a bit more legible to folks with less knowledge of Twisted's internals.
Let suppose that I need work done by doWork function. It returns a deferred for me to hang call backs and error backs on.
d = doWork() d.addCallback(handleWorkDone)
In my handleWorkDone I expect to get the result of doWork completing.
The assert fires if instead of a result value is returned a Deferred is returned. This I consider a bug in the doWork() implementation.
This doesn't sound right. Can you provide an example implementation of doWork that provokes this behavior? Here's an implementation that seems like it matches your description and which does not provoke the behavior: def doWork(): d = Deferred() d.callback("result") return d d = doWork() d.addCallback(handleWorkDone) This doesn't trigger the assert. This calls handleWorkDone with "result". If you simplify the code so the Deferred interaction remains the same but all the extraneous code is removed, it looks like this: d = Deferred() d.callback("result") d.addCallback(handleWorkDone) which *must* work or Deferred is completely useless. Jean-Paul
What must happen in doWork is that it must arrange that any Deferred it used internally has an addCallback used to cause the d returned to the user to complete. Leaking the any internal Deferred() objects must not happen to the user of doWork.
def doWork(): d = Deferred()
def completeWork(result, d): d.callback(result)
inner_d = doAsyncWork() inner_d.addCallback(completeWork, d)
return d
The error message would need to say something like: "Cannot return a Deferred as a result. Did you forgot to addCallback to the deferred?"
Maybe add something to docs based on the above and refer to it in the message?
Barry
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On Wednesday, 27 February 2019 14:45:35 GMT Jean-Paul Calderone wrote:
On Wed, Feb 27, 2019 at 9:34 AM Scott, Barry <barry.scott@forcepoint.com>
wrote:
On Tuesday, 26 February 2019 06:34:28 GMT Glyph wrote:
On Feb 25, 2019, at 3:32 AM, Scott, Barry <barry.scott@forcepoint.com> wrote:>
On Tuesday, 19 February 2019 11:00:57 GMT Chris Withers wrote:
Hi All,
There's this assert: https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.
py# L459
...and I'd like to understand why it's there.
We hit this assert when porting from very old twisted to current
twisted.
In all cases the problem was with our code that used deferreds in a
poor,
not well understood way. After refactoring we are a lot happier with
the
resulting code as it easier to maintain now.
Thanks for the feedback, Barry!
It would still be great to figure out, if we can, how we might make the error message a bit more legible to folks with less knowledge of
Twisted's
internals.
Let suppose that I need work done by doWork function. It returns a deferred for me to hang call backs and error backs on.
d = doWork() d.addCallback(handleWorkDone)
In my handleWorkDone I expect to get the result of doWork completing.
The assert fires if instead of a result value is returned a Deferred is returned. This I consider a bug in the doWork() implementation.
The code I have posted is the good version so it works and the assert does not fire. My goal is to show what I assume is the correct way to code a function that uses internal Deferred(), not give an example that breaks. This should cause the the assert as the code must wait for the thread to return a result. def doWork(): d = Deferred() d2 = deferToThread(doWorkHelper) d.callback(d2) return d Barry
This doesn't sound right. Can you provide an example implementation of doWork that provokes this behavior? Here's an implementation that seems like it matches your description and which does not provoke the behavior:
def doWork(): d = Deferred() d.callback("result") return d
d = doWork() d.addCallback(handleWorkDone)
This doesn't trigger the assert. This calls handleWorkDone with "result". If you simplify the code so the Deferred interaction remains the same but all the extraneous code is removed, it looks like this:
d = Deferred() d.callback("result") d.addCallback(handleWorkDone)
which *must* work or Deferred is completely useless.
Jean-Paul
What must happen in doWork is that it must arrange that any Deferred it used internally has an addCallback used to cause the d returned to the user to complete. Leaking the any internal Deferred() objects must not happen to the user of doWork.
def doWork(): d = Deferred()
def completeWork(result, d): d.callback(result)
inner_d = doAsyncWork() inner_d.addCallback(completeWork, d)
return d
The error message would need to say something like: "Cannot return a Deferred as a result. Did you forgot to addCallback to the deferred?"
Maybe add something to docs based on the above and refer to it in the message?
Barry
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On Wed, Feb 27, 2019 at 10:07 AM Scott, Barry <barry.scott@forcepoint.com> wrote:
The code I have posted is the good version so it works and the assert does not fire. My goal is to show what I assume is the correct way to code a function that uses internal Deferred(), not give an example that breaks.
This should cause the the assert as the code must wait for the thread to return a result.
def doWork(): d = Deferred() d2 = deferToThread(doWorkHelper) d.callback(d2) return d
Ah. I see. Indeed, this code is wrong and triggers the assert. For what it's worth, there is a simpler solution than the example you gave. It uses the "chainDeferred" method: def doWork(): d = Deferred() d2 = deferToThread(doWorkHelper) d2.chainDeferred(d) return d Jean-Paul
On Wednesday, 27 February 2019 15:11:31 GMT Jean-Paul Calderone wrote:
On Wed, Feb 27, 2019 at 10:07 AM Scott, Barry <barry.scott@forcepoint.com>
wrote:
The code I have posted is the good version so it works and the assert does not fire. My goal is to show what I assume is the correct way to code a function that uses internal Deferred(), not give an example that breaks.
This should cause the the assert as the code must wait for the thread to return a result.
def doWork(): d = Deferred() d2 = deferToThread(doWorkHelper) d.callback(d2) return d
Ah. I see. Indeed, this code is wrong and triggers the assert. For what it's worth, there is a simpler solution than the example you gave. It uses the "chainDeferred" method:
def doWork(): d = Deferred() d2 = deferToThread(doWorkHelper) d2.chainDeferred(d) return d
The improved error message could suggest using chainDeferred() maybe.
Jean-Paul
participants (5)
-
Adi Roiban -
Chris Withers -
Glyph -
Jean-Paul Calderone -
Scott, Barry