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

[jack]

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.

[jack]

test case for empty inlineCallbacks function

[jack]

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

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

[exarkun]

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?

[spiv]

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?)

[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?

[exarkun]

Actually, this occurred to me:

def foo():
    yield None

@inlineCallbacks
def bar():
    return foo()

Eh?

[exarkun]

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

[jack]

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

[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).

[exarkun]

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.

[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)

[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.

[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.

[jack]

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

[spiv]

jack: fair enough.

[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.

[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.

[exarkun]

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

[forrestv]

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

[forrestv]

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

[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 {...}

[forrestv]

same as last, but with unit tests!

[ivank]

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

[ivank]

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

2. Use assertRaises instead of failUnlessRaises

3. Docstrings should be written in epytext format and be complete sentences.

4. Docstrings should not refer to "correct [anything]" - mention the exact behavior you're testing for.

5. 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.

6. This needs a news file (see http://twistedmatrix.com/trac/wiki/ReviewProcess )

7. Per coding-standard.xhtml, the maximum line length is 79 characters.

8. Add two blank lines between methods; three blank lines between classes. (Even when it doesn't match old code)

9. Test methods should start with test_, even when it doesn't match old code.

[forrestv]

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

[forrestv]

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

[thijs]

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

[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...

[ivank]

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

[forrestv]

removed copyright change

[forrestv]

Okay, removed.

[therve]

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.

[forrestv]

changed exception message for returnValue

[therve]

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

[therve]

(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.