Opened 2 years ago

Closed 17 months ago

#6164 enhancement closed fixed (fixed)

Returning an already-chained deferred breaks the callback chain

Reported by: radix Owned by: radix
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/circular-deferred-6164-2
(diff, github, buildbot, log)
Author: radix Launchpad Bug:

Description

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.

Change History (21)

comment:1 Changed 2 years ago by radix

  • Author set to radix
  • Branch set to branches/circular-deferred-6164

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

comment:2 Changed 2 years ago by radix

  • Owner set to radix
  • Status changed from new to assigned

comment:3 Changed 2 years ago by radix

  • Keywords review added
  • Owner radix deleted
  • Status changed from assigned to new

comment:4 Changed 2 years ago by 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).

comment:5 Changed 2 years ago by therve

  • Keywords review removed
  • Owner set to radix

Cool. Please add a NEWS fragment and merge.

comment:6 Changed 2 years ago by therve

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

comment:7 Changed 18 months ago by radix

  • Keywords review added
  • Owner radix deleted

I've added a paragraph.

comment:8 Changed 18 months ago by radix

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

comment:9 Changed 18 months ago by therve

  • Keywords review removed
  • Owner set to radix

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

comment:10 Changed 18 months ago by radix

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

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

comment:11 Changed 18 months ago by 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.


comment:12 Changed 18 months ago by tomprince

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:13 Changed 18 months ago by tom.prince

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

comment:14 Changed 18 months ago by radix

  • Branch changed from branches/circular-deferred-6164 to branches/circular-deferred-6164-2

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

comment:15 Changed 18 months ago by radix

  • Keywords review added
  • Owner radix deleted
  • Status changed from reopened to new

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.

comment:16 Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to radix
  1. (optional) In the howto, do we just want to say this will eventually be an exception, rather than only may be? If we plan to do this, we shouldn't hedge. If not, we shouldn't mention it.
  2. Sorry for flip-flopping on this, but reading the current news file, this sounds more like a feature, then removing something.

Please merge after addressing the above points.

comment:17 Changed 17 months ago by radix

  1. fixed
  2. I don't really agree. I think as a rule, new deprecations should be in the deprecations & removals section, so that people can look there to see what they may need to do to fix their code.

comment:18 Changed 17 months ago by radix

[21:59] <tos9> I would vote for removal.
[21:59] <tos9> Feature seems more reasonable for ... things that are notable. The part that's notable here is just "don't do that"

I'm gonna just merge this in as a removal.

comment:19 Changed 17 months ago by radix

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

(In [39048]) Merge circular-deferred-6164-2: Emit a DeprecationWarning for "circular" Deferreds

Author: radix
Reviewers: tom.prince, therve
Fixes: #6164

When a Deferred is returned from one of its own callbacks, a
DeprecationWarning will now be emitted, since this currently
leads to a "broken" Deferred -- one which has no result and will
never fire another callback.

comment:20 Changed 17 months ago by radix

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [39072]) Revert r39048: (circular-deferred-6164-2) introduced pyflakes warnings

twisted/test/test_defer.py:916: redefinition of unused 'warnings' from line 10

Reopens: #6164

comment:21 Changed 17 months ago by radix

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

(In [39074]) Merge circular-deferred-6164-2: Emit a DeprecationWarning for "circular" Deferreds

Author: radix
Reviewers: tom.prince, therve
Fixes: #6164

When a Deferred is returned from one of its own callbacks, a
DeprecationWarning will now be emitted, since this currently
leads to a "broken" Deferred -- one which has no result and will
never fire another callback.

Note: See TracTickets for help on using tickets.