non-local inlineCallbacks exit from returnValue being called in the wrong function is very confusing

[glyph]

Let's say we have a chunk of code like this:

from twisted.internet.defer import inlineCallbacks, returnValue, succeed
def asyncOperation():
    return succeed(1)
@inlineCallbacks
def thunk():
    value = yield asyncOperation()
    if value:
        returnValue(value)
@inlineCallbacks
def main():
    print "Result:", (yield thunk())
main()

This is reasonably straightforward. However, let's say we wanted to refactor it, and we end up with this definition of 'thunk':

def thunk2(value):
    if value:
        returnValue(value)
@inlineCallbacks
def thunk():
    value = yield asyncOperation()
    returnValue((yield thunk2(value)) + 1)

(I realize that at such a small scale, a copy/paste error of this type seems ludicrous, but in a larger function, especially if it's spread across many functions and methods decorated with @inlineCallbacks, it's quite feasible.)

The second version exits immediately with the "wrong" value, and it's very difficult to tell why. Worse yet, it fails even if remember to decorate thunk2 with inlineCallbacks; it only starts to give us the "right" value when thunk2 is properly a generator and decorated. Debugging this is tricky, because it looks like returnValue is causing thunk to exist, just with the wrong value!

[glyph]

exarkun and I independently thought that a potential solution to this problem would be to check the stack in returnValue to make sure that we're invoked directly from a correctly-decorated function, and emit a warning otherwise. Unfortunately this might be quite slow, and therefore we probably only want to turn it on when Deferred.debug is True.

[glyph]

I also thought that we might be able to magically replace returnValue in the decorated function's func_globals. I think that might be faster, but that wouldn't work with different invocation styles (e.g. "defer.returnValue(...)" or "twisted.internet.defer.returnValue(...)").

[glyph]

(In [27845]) Branching to 'nonlocal-4157'

[glyph]

I didn't bother to make the code optional or debug-only, because we don't have any benchmarks so I'm not even sure what the impact is. It seems to me that this is unlikely to be a hotspot, since Deferreds already have some level of overhead, and the extra cost here in the case where you are calling returnValue from the right stack frame is only:

• native function call
• tuple item access
• boolean test
• 3 attribute accesses

which seems pretty low even just compared to the shenanigans that invoking the callback chain will get up to.

[radix]

1. Did you intentionally use warn_explicit instead of warn to emit the warning? I can only imagine you're trying to make it faster, but this is in the "bad" case anyway, so I see no reason. I think that if you want to reduce the performance hit of this code, you should put it behind a warning flag, not resort to undocumented features of the warning system (we have enough of those as it stands).

2. There's a typo in NonLocalExitTests' docstring:

    It's possible for Le{returnValue} to be (accidentally) invoked at a stack

s/Le/L/

3. I think the code change here requires a lot more commenting. In particular, this line:

+            if appCodeTrace.tb_next.tb_next:

4. I think the warning should be a bit more informative, especially since it's a deprecation. It should explain that returnValue should only be called in an inlineCallbacks-using function.

[glyph]

Thanks for the review.

Replying to radix:

1. Did you intentionally use warn_explicit instead of warn to emit the warning?

Yes.

I can only imagine you're trying to make it faster, but this is in the "bad" case anyway, so I see no reason.

I assure you, performance had absolutely nothing to do with it :).

warnings.warn takes a "stacklevel", which is a number of levels up the stack from the current call frame that is responsible for the warning, i.e. the frame to use for filename/lineno. If you read the code in the branch carefully, you'll notice that I am emitting the warning for a level down the stack, for the no-longer-executing frame which called returnValue. I can determine the filename and line number myself, but there's no value for "stacklevel" which would give correct results. warn_explicit is the only way to pass those parameters... well, explicitly.

I think that if you want to reduce the performance hit of this code, you should put it behind a warning flag, not resort to undocumented features of the warning system (we have enough of those as it stands).

warn_explicit is perfectly well documented, and has been for quite a while:

•  http://docs.python.org/library/warnings.html#warnings.warn_explicit
•  http://www.python.org/doc/2.5.4/lib/warning-functions.html#l2h-5207
•  http://www.python.org/doc/2.4.4/lib/warning-functions.html#l2h-690

2. There's a typo in NonLocalExitTests' docstring: {{{ It's possible for Le{returnValue} to be (accidentally) invoked at a stack }}}

Fixed.

s/Le/L/ 3. I think the code change here requires a lot more commenting. In particular, this line: {{{ + if appCodeTrace.tb_next.tb_next: }}}

I added a ton of explanation to the function; you're right, stack manipulation is one of those things that should always be explained in detail.

4. I think the warning should be a bit more informative, especially since it's a deprecation. It should explain that returnValue should only be called in an inlineCallbacks-using function.

Okay, done. I think that makes it a little more awkward, but someone getting this warning is going to need all the help that they can get.

[TimAllen]

I'm not exceedingly confident with stack-munging and decorators, but I do have some comments:

1. The warning mentions the filename and lineno of the returnValue() that is causing the breakage, but (I think) it only mentions the name of the function that's being broken. I can imagine myself diagnosing such a problem in a production system and wanting to figure out which of the various methods named, say, render_GET() were affected, to gauge the severity of the issue.
2. Oh no, another Python-2.5-only source file! I spent a lot of time wrestling with twisted/test/generator_failure_tests.py in ticket #1696, don't make me do the same thing all over again with twisted/internet/test/inlinecb_tests.py too! For the record, my patch in #1696 made tests 2.4-safe like this:

   class GeneratorFailureTestCase(unittest.TestCase):
       """
       Python 2.5 test cases for failures thrown into generators.
       """
   
   
       if sys.version_info[0] <= 2 and sys.version_info[1] <= 4:
           # Running these tests will raise all kinds of errors on versions before
           # 2.5, so skip the whole lot. Note that examples of syntax introduce in
           # 2.5 have to be wrapped in exec so that this file doesn't cause Python
           # 2.4 to die with a SyntaxError when it tries to parse the file.
           skip = "generator_failure_tests require Python 2.5"
   
   
       def test_throwExceptionIntoGenerator(self):
           """
           It should be possible to throw the exception that a Failure
           represents into a generator.
           """
           stuff = []
           exec """
               def generator():
                   try:
                       yield
                   except:
                       stuff.append(sys.exc_info())
                   else:
                       self.fail("Yield should have yielded exception.")
               """
           g = generator()
           f = getDivisionFailure()
           g.next()
           f.throwIntoGenerator(g)
   
           self.assertEquals(stuff[0][0], ZeroDivisionError)
           self.assertTrue(isinstance(stuff[0][1], ZeroDivisionError))
   
           self.assertEquals(traceback.extract_tb(stuff[0][2])[-1][-1], "1/0")
   

3. Both tests seem to have identical docstrings.
4. You need a news file describing this change. :)

[glyph]

Replying to TimAllen:

I'm not exceedingly confident with stack-munging and decorators, but I do have some comments:

Thanks for the review!

1. The warning mentions the filename and lineno of the returnValue() that is causing the breakage, but (I think) it only mentions the name of the function that's being broken. I can imagine myself diagnosing such a problem in a production system and wanting to figure out which of the various methods named, say, render_GET() were affected, to gauge the severity of the issue.

I think the tradeoff right now is reasonable. If we wanted to emit the line number from both functions, we'd have to emit two warnings, which would be potentially confusing. The right thing for the programmer to do when they see this warning is to immediately go and either decorate the function emitting the warning with @inlineCallbacks, or delete the call to returnValue. The function being exited isn't terribly interesting, because you don't need to touch it to fix the problem.

In a future release, when this becomes an exception instead of a warning, you'll have the whole stack trace.

2. Oh no, another Python-2.5-only source file! I spent a lot of time wrestling with twisted/test/generator_failure_tests.py in ticket #1696, don't make me do the same thing all over again with twisted/internet/test/inlinecb_tests.py too! For the record, my patch in #1696 made tests 2.4-safe like this:

exec is bad, kids. Stay in school.

Ahem. Seriously, exec makes code invisible to syntax highlighting, to pyflakes, to any IDE that tries to inspect what's going on, and generally amkes working with the code more difficult. We should have a better way of dealing with new-syntax test fixtures, especially as we may eventually have uses for other new-syntax facilities.

I filed #4182 to track this, and we should have a general way of dealing with the problem. This is perhaps closely related to #4138.

3. Both tests seem to have identical docstrings.

I cleaned up the docstrings a bunch, and factored out some duplication so that hopefully now it's clear what's being tested by each method.

4. You need a news file describing this change. :)

Done.

[jesstess]

Some feedback:

1. The docstrings, internal documentation in _inlineCallbacks, and unit tests were very clear about the problem and solution, even for someone unfamiliar with the issue before reviewing this ticket, aka me.
2. bump copyrights in all 3 files.
3. In defer.py: '"with inlineCallbacks"% (' ==> '"with inlineCallbacks" % ('
4. buildbot had no history on this branch, so I ran a  build and it looks good.

To the best of my knowledge this branch is functionally correct and ready to merge modulo the points above.

[glyph]

Replying to jesstess:

Some feedback:

1. The docstrings, internal documentation in _inlineCallbacks, and unit tests were very clear about the problem and solution, even for someone unfamiliar with the issue before reviewing this ticket, aka me.

Thanks! I'm really glad to hear that. This issue is really nasty and subtle, and I was afraid that it might be incomprehensible to someone who received the warning.

2. bump copyrights in all 3 files.

Done.

1. In defer.py: '"with inlineCallbacks"% (' ==> '"with inlineCallbacks" % ('

Oops; Done.

1. buildbot had no history on this branch, so I ran a  build and it looks good.

Thanks, that's great.

Looks like we have some other 2.4-related issues on trunk though...

To the best of my knowledge this branch is functionally correct and ready to merge modulo the points above.

Thanks very much for the review. Merge impending.

[glyph]

(In [27927]) Fix returnValue to warn you if it causes a non-local exit.

If returnValue is invoked outside of a function decorated with @inlineCallbacks, but causes a function thusly decorated to exit, a DeprecationWarning will be emitted explaining this potentially confusing behavior. In a future release, this will cause an exception.

Author: glyph

Reviewer: radix, TimAllen, jesstess

Fixes #4157