Opened 8 years ago

Closed 3 years ago

#2501 defect closed fixed (fixed)

Make defer.inlineCallbacks raise an exception if it has a non-generator

Reported by: jack Owned by: forrestv
Priority: normal Milestone:
Component: core Keywords: defer
Cc: twonds, exarkun, spiv, radix, ivank-twisted-bugs@…, thijs Branch: branches/inlinecallbacks-nongen-2501
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description (last modified by thijs)

If defer.inlineCallbacks is used on an empty function, the function will generate an exception. Test case attached which demonstrates the error. The testcase requires Python 2.5 to run since it depends on defer.inlineCallbacks.

I ran into this while stubbing out test cases for XMPP components.

Attachments (8)

test_inline.py (352 bytes) - added by jack 8 years ago.
test case for empty inlineCallbacks function
inlineCallbacks.patch (870 bytes) - added by exarkun 6 years ago.
inlineCallbacks_not_generator.diff (765 bytes) - added by forrestv 3 years ago.
patch to raise a more informative exception when a non-generator is supplied to inlineCallbacks
inlineCallbacks_not_generator2.diff (962 bytes) - added by forrestv 3 years ago.
same as last, but catches exceptions too (especially the tricky _DefGen_Return() one)
inlineCallbacks_not_generator3.diff (2.2 KB) - added by forrestv 3 years ago.
same as last, but with unit tests!
inlineCallbacks_not_generator4.diff (2.8 KB) - added by forrestv 3 years ago.
save as last, but with style fixes and only catching _DefGen_Return
inlineCallbacks_not_generator5.diff (2.5 KB) - added by forrestv 3 years ago.
removed copyright change
inlineCallbacks_not_generator6.diff (2.5 KB) - added by forrestv 3 years ago.
changed exception message for returnValue

Download all attachments as: .zip

Change History (41)

Changed 8 years ago by jack

test case for empty inlineCallbacks function

comment:1 Changed 8 years ago by jack

I should also note that a workaround is to stub function like this:

@defer.inlineCallbacks
def iAmEmpty(self):
    yield defer.succeed(None)

comment:2 Changed 8 years ago by exarkun

  • Cc exarkun added
  • Owner changed from glyph to radix

inlineCallbacks can't decorate any non-generator function. It's a decorator for generator functions, after all. :)

Perhaps this needs to be documented more clearly?

comment:3 Changed 7 years ago by spiv

  • Cc spiv added

It's fairly easy to make a decorator introspect the object it's decorating to see if it's a generator or not. How about we make inlineCallbacks raise an exception if the function isn't generator function? (Or if it's considered too obnoxious to raise exceptions at import time, emit a warning?)

comment:4 Changed 6 years ago by exarkun

an exception at import time would be great. how do you propose determining if a function is a generator function? inspect the code object's flags? will this introduce an incompatibility with other python runtimes?

comment:5 Changed 6 years ago by exarkun

Actually, this occurred to me:

def foo():
    yield None

@inlineCallbacks
def bar():
    return foo()

Eh?

comment:6 Changed 6 years ago by exarkun

So maybe the documentation for inlineCallbacks should just be more clear?

comment:7 Changed 6 years ago by jack

Improving the documentation seems fair enough to me, especially as it sounds like making the exception better is fairly hard.

Changed 6 years ago by exarkun

comment:8 Changed 6 years ago by exarkun

  • Cc radix added
  • Keywords review added
  • Owner radix deleted

comment:9 Changed 6 years ago by TimothyFitz

I believe a documentation change is wrong, since users have a clear expectation of behavior, and even after knowing the existing behavior it is easy to mistakenly wrap a function.

I want both of these cases to work:

def foo():
    yield None
    returnValue(42)

@inlineCallbacks
def bar():
    return foo()

and

@inlineCallbacks
def foo():
    return 42

where the expected result in both is the same, a deferred which succeeds with the value 42.

The way to make both of these work and retain close to full backwards compatibility is to have inlineCallbacks be spelled:

def inlineCallbacks(f)
    def unwindGenerator(*args, **kwargs):
        try:
            result = f(*args, **kwargs)
        except Exception:
            return defer.fail()

        if isinstance(result, GeneratorType):
            return _inlineCallbacks(None, result, Deferred())
        else:
            return defer.succeed(result)

    return mergeFunctionMetadata(f, unwindGenerator)

(potential implementation to convey nuances of error handling, not necessarily what final code will be)

The documentation can explain that inlineCallbacks wraps a function which returns a generator, and handles the case where it's an immediate result by simply returning defer.fail() or defer.succeed().

The existing case this breaks is when the wrapped function returns a generator-like instance. It will now return defer.succeed(myGeneratorLikeObject). We may want to expose _inlineCallbacks (My assumptionis that this case is uncommon, this may be false).

comment:10 Changed 6 years ago by exarkun

  • Keywords review removed

I don't really see a lot of value in the latter case. inlineCallbacks is providing no value there; supporting that case is only useful inasmuch as it lets one go between generator and non-generator without removing the decorator. Is that worth anything? Perhaps. Is it worth enough to justify the complexity? I don't know.

I rarely use inlineCallbacks so I can't say whether I would find this change useful or not. Dropping the review keyword because apparently there is more discussion to be had on this ticket.

comment:11 Changed 6 years ago by TimothyFitz

if

def foo():
    yield None
    returnValue(42)

@inlineCallbacks
def bar():
    return foo()

is a non-case, this change simplifies dramatically, just add this guard

if isinstance(f, FuncutionType): 
    return defer.maybeDeferred(f, args, kwargs)

comment:12 Changed 6 years ago by jack

We use inlineCallbacks for everything, and when writing trial test cases we had ot make our stub functions more complex by putting in yields, instead of them just having simple returns. So for writing tests, I think being able to go back and forth is very useful.

Timothy's solution looks simple and easy to me.

comment:13 follow-up: Changed 6 years ago by spiv

(Apologies for the long rambling comment...)

I'm -1 on making inlineCallbacks work with non-generators. It will mask mistakes. My preferred behaviour is definitely that it should immediately blow up (as I just realised I already commented on this ticket several months ago :)

TimothyFitz: As a user, I do have a clear expectation of behavior for inlineCallbacks, and that is that it should only work with generator functions :)

@inlineCallbacks is a way to write code that uses and returns Deferreds using generator function syntax. It's not a way to do that, plus act like maybeDeferred if it's not a generator, plus a way to wrap a generator not returned by a function, plus a way to make my toast in the mornings...

I strongly suspect that the cases being discussed in this ticket are not common enough to justify making inlineCallbacks so lax that it is easy to do the wrong thing, and do so in a such a way that you don't notice until runtime.

So, I'd say it should check at import time that the function is a generator, i.e. "f.func_code.co_flags & 32 != 0", although maybe only if running in CPython. (I'm not sure how portable the co_flags values are.)

I'd be fine with providing an alternative API, called @inlineCallbacksMagic or something, that does the best-guess DWIM behaviours people are discussing in this ticket. But I think @inlineCallbacks should be strict by default. I think it's easier to document, teach and debug that way.

If it turns out that inlineCallbacksMagic is a complete superset of the API of inlineCallbacks, and that we wished that inlineCallbacks did that, then great. It's easy to change inlineCallbacks to become more relaxed in a future release. It's much harder to make an existing API stricter in a backwards-compatible way than to relax it. So please, let us try out any new more magic variants of inlineCallbacks in a new decorator, rather than immediately changing the API we've got.

So, I propose we:

  • add the co_flags check at import time
  • clarify the inlineCallbacks documention
  • invite TimothyFitz (or anyone else interested) to propose a new, alternative API with the behaviours they want. I don't want to set the barrier prohibitively high, but the ideal form for this would be a patch with tests demonstrating the behaviours. They should try using it in real code and let us know how it works out for them. We can then decide to add it to Twisted, or change inlineCallbacks after all, or whatever makes sense.

My thinking on the last point there is that it doesn't seem hard to write these inlineCallbacks variations that have been discussed. So someone ought to write it, use it in their own code, and get some concrete experience with it.

comment:14 Changed 6 years ago by jack

If you want it to blow up, that's fine, but please make it blow up with a useful error message.

comment:15 Changed 6 years ago by spiv

jack: fair enough.

comment:16 Changed 6 years ago by exarkun

  • Owner set to TimothyFitz

comment:17 in reply to: ↑ 13 Changed 6 years ago by TimothyFitz

Replying to spiv:

I'm -1 on making inlineCallbacks work with non-generators. It will mask mistakes. My preferred behaviour is definitely that it should immediately blow up (as I just realised I already commented on this ticket several months ago :)

Can you give me an example of a mistake it masks? Every single "mistake" I make could be handled intuitively by an API.

I'm -1 on adding a new API, since we already have two (defgen and inlineCallbacks), I don't really want a third spelling.

comment:18 Changed 6 years ago by exarkun

I'm -1 on adding a new API, since we already have two (defgen and inlineCallbacks), I don't really want a third spelling.

We don't already have two APIs. Twisted has thousands of APIs, and we add new ones all the time. So adding an API for introspectively selecting between deferredGenerator/inlineCallbacks/maybeDeferred/making toast is hardly a burden.

I'm not really in favor or opposed to having inlineCallbacks raise an exception at call time if passed a non-generator function. I'd still be happy with a documentation-only fix for this. If someone wants to enhance inlineCallbacks by having it determine if it is decorating a non-generator function and raising an exception then, that's cool - please file a separate ticket and do it there.

comment:19 Changed 5 years ago by exarkun

Anyone feel like implementing the flags check described by spiv above?

Changed 3 years ago by forrestv

patch to raise a more informative exception when a non-generator is supplied to inlineCallbacks

Changed 3 years ago by forrestv

same as last, but catches exceptions too (especially the tricky _DefGen_Return() one)

comment:20 Changed 3 years ago by forrestv

inlineCallbacks produces incredibly unhelpful exceptions when more than an empty function is used. This exception is from the usage of returnValue in a non-generator (yield was commented out) inside another function using inlineCallbacks.

Unhandled error in Deferred:
Traceback (most recent call last):
  File "/home/forrest/pylib/twisted/web/http.py", line 803, in requestReceived
    self.process()
  File "/home/forrest/pylib/twisted/web/server.py", line 125, in process
    self.render(resrc)
  File "/home/forrest/pylib/twisted/web/server.py", line 132, in render
    body = resrc.render(self)
  File "/home/forrest/repos/forre.st/p2pool/util.py", line 15, in render
    defer.maybeDeferred(resource.Resource.render, self, request).addCallbacks(finish, finish_error)
--- <exception caught here> ---
  File "/home/forrest/pylib/twisted/internet/defer.py", line 133, in maybeDeferred
    result = f(*args, **kw)
  File "/home/forrest/pylib/twisted/web/resource.py", line 210, in render
    return m(request)
  File "/home/forrest/pylib/twisted/internet/defer.py", line 1141, in unwindGenerator
    return _inlineCallbacks(None, f(*args, **kwargs), Deferred())
  File "/home/forrest/pylib/twisted/internet/defer.py", line 1046, in _inlineCallbacks
    if appCodeTrace.tb_next.tb_next:
exceptions.AttributeError: 'NoneType' object has no attribute 'tb_next'

The above patch creates a more informative error:

Traceback (most recent call last):
  File "/home/forrest/repos/forre.st/p2pool/jsonrpc.py", line 113, in render_POST
    result = yield df
TypeError: inlineCallbacks requires <function rpc_getwork at 0x28a18c0> to produce a generator; instead caught _DefGen_Return()

It also works for functions that return a value:

Traceback (most recent call last):
  File "/home/forrest/repos/forre.st/p2pool/jsonrpc.py", line 113, in render_POST
    result = yield df
TypeError: inlineCallbacks requires <function rpc_getwork at 0x2ca6848> to produce a generator; instead got {...}

Changed 3 years ago by forrestv

same as last, but with unit tests!

comment:21 Changed 3 years ago by ivank

  • Cc ivank-twisted-bugs@… added
  • Keywords review added
  • Summary changed from defer.inlineCallbacks can't decorate an empty function to Make defer.inlineCallbacks raise an exception if it has a non-generator

Adding a review keyword because it's probably supposed to be there

comment:22 Changed 3 years ago by ivank

  • Keywords review removed
  • Owner changed from TimothyFitz to forrestv

forrestv, thanks for working on this!

  1. I don't think this should raise TypeError for any exception it catches. This probably violates backwards-compatibility policy. For example, this should probably continue working the same:
from twisted.internet import defer

def someGenerator():
        yield 3

def a():
        1/0
        return someGenerator()

b = defer.inlineCallbacks(a)

###

>>> b()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/.../twisted/internet/defer.py", line 1145, in unwindGenerator
    return _inlineCallbacks(None, f(*args, **kwargs), Deferred())
  File "generator.py", line 7, in a
    1/0
ZeroDivisionError: integer division or modulo by zero
  1. Use assertRaises instead of failUnlessRaises
  1. Docstrings should be written in epytext format and be complete sentences.
  1. Docstrings should not refer to "correct [anything]" - mention the exact behavior you're testing for.
  1. Because so many things can raise TypeError, you should check the message of exception. This isn't hard because trial's assertRaises returns the exception object. Check the str(exc) to make sure you have the right message.
  1. This needs a news file (see http://twistedmatrix.com/trac/wiki/ReviewProcess )
  1. Per coding-standard.xhtml, the maximum line length is 79 characters.
  1. Add two blank lines between methods; three blank lines between classes. (Even when it doesn't match old code)
  1. Test methods should start with test_, even when it doesn't match old code.

Changed 3 years ago by forrestv

save as last, but with style fixes and only catching _DefGen_Return

comment:23 Changed 3 years ago by forrestv

Okay, I think I fixed all that with 'inlineCallbacks_not_generator4.diff'. (:

comment:24 Changed 3 years ago by thijs

  • Cc thijs added
  • Description modified (diff)

The year reference in the copyright header of twisted/internet/defer.py needs to go.

comment:25 Changed 3 years ago by forrestv

Does it? coding-standard.xhtml says:

When you update existing files, make sure the year in the copyright
header is up to date as well. You should add a new copyright header when
it's completely missing in the file that is being edited.

I thought it was a bit weird but decided that I should follow it...

comment:26 Changed 3 years ago by ivank

A few months ago, the copyright years in the headers were permanently removed. coding-standard.xhtml needs to be fixed. I filed #5112

Changed 3 years ago by forrestv

removed copyright change

comment:27 Changed 3 years ago by forrestv

Okay, removed.

comment:28 Changed 3 years ago by forrestv

  • Keywords review added

comment:29 Changed 3 years ago by forrestv

  • Owner changed from forrestv to jesstess

comment:30 Changed 3 years ago by jesstess

  • Owner jesstess deleted

comment:31 Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to forrestv

Looks good! Just one comment:

  • The error message shouldn't mention _DefGen_Return which is private, but returnValue instead.

I'd be happy to apply once fixed.

Changed 3 years ago by forrestv

changed exception message for returnValue

comment:32 Changed 3 years ago by therve

  • Author set to therve
  • Branch set to branches/inlinecallbacks-nongen-2501

(In [32161]) Branching to 'inlinecallbacks-nongen-2501'

comment:33 Changed 3 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [32163]) Merge inlinecallbacks-nongen-2501

Author: forrestv
Reviewer: therve
Fixes: #2501

inlineCallbacks now raises TypeError when the decorated function returns
something other than a generator or uses returnValue as a non-generator.

Note: See TracTickets for help on using tickets.