Mailman 3 [Twisted-Python] why can't a callback be called with a deferred? - Twisted

[Twisted-Python] why can't a callback be called with a deferred?

Chris Withers

19 Feb 2019 19 Feb '19

11 a.m.

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

Show replies by date

Adi Roiban

19 Feb 19 Feb

11:41 a.m.

On Tue, 19 Feb 2019 at 11:01, Chris Withers 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

Chris Withers

1:34 p.m.

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

Glyph

21 Feb 21 Feb

6:55 a.m.

...

On Feb 19, 2019, at 5:34 AM, Chris Withers 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

Chris Withers

7:03 a.m.

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

Scott, Barry

25 Feb 25 Feb

11:32 a.m.

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

Glyph

26 Feb 26 Feb

6:34 a.m.

...

On Feb 25, 2019, at 3:32 AM, Scott, Barry 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

Scott, Barry

27 Feb 27 Feb

2:34 p.m.

On Tuesday, 26 February 2019 06:34:28 GMT Glyph wrote:

...

...
On Feb 25, 2019, at 3:32 AM, Scott, Barry 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

Jean-Paul Calderone

2:45 p.m.

On Wed, Feb 27, 2019 at 9:34 AM Scott, Barry wrote:

...

...
...
On Feb 25, 2019, at 3:32 AM, Scott, Barry 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

Scott, Barry

3:06 p.m.

On Wednesday, 27 February 2019 14:45:35 GMT Jean-Paul Calderone wrote:

...

On Wed, Feb 27, 2019 at 9:34 AM Scott, Barry

wrote:

...
On Tuesday, 26 February 2019 06:34:28 GMT Glyph wrote:

...
...
On Feb 25, 2019, at 3:32 AM, Scott, Barry 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

Jean-Paul Calderone

3:11 p.m.

On Wed, Feb 27, 2019 at 10:07 AM Scott, Barry 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

Scott, Barry

28 Feb 28 Feb

10:08 a.m.

On Wednesday, 27 February 2019 15:11:31 GMT Jean-Paul Calderone wrote:

...

On Wed, Feb 27, 2019 at 10:07 AM Scott, Barry

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

1883

Age (days ago)

1892

Last active (days ago)

List overview

Download

11 comments

5 participants

participants (5)

Adi Roiban
Chris Withers
Glyph
Jean-Paul Calderone
Scott, Barry

[Twisted-Python] why can't a callback be called with a deferred?

Adi Roiban

tags

participants (5)