Returning an already-chained deferred breaks the callback chain

[radix]

If d1 has a callback that returns d1, the callback chain will halt, meaning no further callbacks on that Deferred will ever be run (as far as I can tell; I haven't figured out any other way to un-stick the Deferred).

We should warn the user in this case.

That's the easy one. You can also do an indirect Deferred chain -- d1 returning d2 from a callback which itself has a callback that returns d1. This continues to "work", but it's probably still always a bad thing to do. If we can warn the user in this case too, that'd be nice.

[radix]

(In [36281]) Branching to 'circular-deferred-6164'

[radix]

The branch under review currently only detects *direct* circular chains, when d1 has a callback that returns d1.

I haven't tried very hard to detect the indirect case, but it looks like it won't be trivial (it looks like chained deferreds basically form a linked list, with outer -> inner, but no reverse link, so it will be hard to traverse backwards without changing this structure).

[therve]

Cool. Please add a NEWS fragment and merge.

[therve]

After some other consideration, please also add a small section and an example in the doc (doc/core/howto/defer.xhtml probably). Thanks!

[radix]

I've added a paragraph.

[radix]

Everything should be in order. I forced a build on buildbot and only got spurious unrelated errors.

[therve]

Cool. Please pass a custom message to your assertTrue so that we can get a good error if it fails. Please merge once fixed.

[radix]

(In [38513]) Merge circular-deferred-6164

Author: radix Reviewer: therve Fixes: #6164

If a callback that is attached to a Deferred returns that same Deferred, a warning will now be emitted. Deferred howto documentation has been updated.

[exarkun]

For what it's worth, I think this is an incorrect use of the warnings system. Can you explain why test_circularChainWarning should be blamed for this condition? It has nothing to do with the problem being reported, it just happened to fire the Deferred. I can see two alternate possibilities that make more sense:

1. circularCallback should be blamed. This seems to make the most sense, since circularCallback is the callback function that returned the "circular" Deferred.
2. test_circularChainWarning should be blamed because it *called* d.addCallback(circularCallback). I don't think this is as good as (1) and it's harder to implement, so there doesn't seem to be much to recommend it. The only thing it has going for it is that the test accidentally passes even though the implementation is incorrect (hm, actually that doesn't sound very positive).

Please think about the intent of the argument accepted by flushWarnings when testing and implementing warnings. If you give it the right value, it's quite useful, but if you give it the wrong one then it's going to force warnings to be pointed at nonsense locations.

In case it helps, the way to easily implement the behavior described in (1) is with twisted.python.deprecate.warnAboutFunction.

[tomprince]

(In [38762]) Revert r38513 - python 3.3 regression

Reopens: #6164

FAIL: test_circularChainWarning (twisted.test.test_defer.DeferredTestCase)
test_circularChainWarning
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/lib/buildbot/buildarea/python-3.3-tests/Twisted/twisted/trial/_synctest.py", line 1211, in _run
    runWithWarningsSuppressed(suppress, method)
  File "/var/lib/buildbot/buildarea/python-3.3-tests/Twisted/twisted/python/util.py", line 1065, in runWithWarningsSuppressed
    return f(*args, **kwargs)
  File "/var/lib/buildbot/buildarea/python-3.3-tests/Twisted/twisted/test/test_defer.py", line 922, in test_circularChainWarning
    "\nExpected match: %r\nGot: %r" % (pattern, warning['message']))
  File "/var/lib/buildbot/buildarea/python-3.3-tests/Twisted/twisted/trial/_synctest.py", line 308, in assertTrue
    raise self.failureException(msg)
twisted.trial.unittest.FailTest:
Expected match: 'Callback <function circularCallback at 0x\\w+> returned the same Deferred it was attached to'
Got: 'Callback <function DeferredTestCase.test_circularChainWarning.<locals>.circularCallback at 0x7fa10ee5ed40> returned the same Deferred it was attached to; this breaks the callback chain and will raise an exception in the future.'

It appears that this is simply an issue with reprs of local functions.

[tom.prince]

Also, since this is introducing a deprecation warning, it probably warrants a .removal.

[radix]

(In [38763]) Branching to 'circular-deferred-6164-2'.

[radix]

All issues have been addressed, including the ones raised in #6534. I've also added a .removal instead of a .misc.

This will need a rereview, since I merged in the changes from #6534 which wasn't fully reviewed yet.