#6534 defect closed duplicate (duplicate)

The circular deferred warning is not as specific and informative as it should be

Reported by: radix Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/improve-circular-warning-6534
(diff, github, buildbot, log)
Author: radix Launchpad Bug:

Description

As described by exarkun on http://twistedmatrix.com/trac/ticket/6164#comment:11

The warning message describes the function that is in error, but the function that the warning is *bound* to is the one that invoked Deferred.callback, which is not the code in error.

Change History (11)

comment:1 Changed 17 months ago by radix

  • Author set to radix
  • Branch set to branches/improve-circular-warning-6534

(In [38544]) Branching to 'improve-circular-warnings-6534'

comment:2 Changed 17 months ago by radix

  • Keywords review added
  • Owner radix deleted

comment:3 Changed 17 months ago by thijs

  • Cc thijs added

This is not a review but wanted to point out a recent failing test on trunk for python3.3 that might be fixed with this ticket:

======================================================================
FAIL: test_circularChainWarning (twisted.test.test_defer.DeferredTestCase)
test_circularChainWarning
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Twisted/trunk/twisted/trial/_synctest.py", line 1211, in _run
    runWithWarningsSuppressed(suppress, method)
  File "/Twisted/trunk/twisted/python/util.py", line 1065, in runWithWarningsSuppressed
    return f(*args, **kwargs)
  File "/Twisted/trunk/twisted/test/test_defer.py", line 922, in test_circularChainWarning
    "\nExpected match: %r\nGot: %r" % (pattern, warning['message']))
  File "/Twisted/trunk/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 0x7f402a674320> returned the same Deferred it was attached to; this breaks the callback chain and will raise an exception in the future.'

----------------------------------------------------------------------
Ran 3195 tests in 7.826s

FAILED (failures=1, skipped=952)

comment:4 Changed 17 months ago by exarkun

  • Keywords review removed
  • Owner set to radix

Thanks! A couple minor things:

  1. It seems like the test needn't use a regular expression anymore, since the warning now has fixed text in its message.
  2. I am thinking about the consequences of -Werror on code that encounters this warning. One good thing is that it will cause an exception to pop out of somewhere and hopefully the developer can fix their code. However... I think it'll pop out of _runCallbacks, therefore out of callback or addCallback, which is super surprising. Thinking beyond that, after we replace this deprecation with an exception, do we want that exception coming out of _runCallbacks? I suspect that would ruin a lot of code. So perhaps the warning belongs where -Werror will cause it to raise an exception roughly like the one that the warning will eventually be replaced with. (All that said, I'm not sure anyone actually takes advantage of -Werror in this way, so it may not matter - but it seems worth considering, at least, particularly since if we don't think about this now we're still going to have to think about it later, after we've committed ourselves by introducing this warning. Also, I think it's interesting to think about the consequences of leaving _runCallbacks via an exception raised at this point: what state will the Deferred being processed be left in? What state will the Deferreds in chain be left in?)

Thanks. Please merge after addressing this points to your satisfaction.

comment:5 Changed 17 months ago by radix

#1 fixed

#2 there are several points here.

2a. -Werror causing exceptions being raised from callback() or addCallback() is surprising

2b. raising an explicit exception from _runCallbacks, when we turn this warning into an exception, would ruin code

2c. something about the exception being raised from the same place that -Werror would cause it to be raised.

2d. maybe raising out of _runCallbacks will cause weird state issues

To address them:

2a. I'm not really sure where else it could be raised, so I guess it doesn't seem surprising to me. Doing anything particularly clever here to deal with the issue seems not very worthwhile, since anyone who's encountering this has buggy code anyway, and they will be notified of it when the buggy code is executed, one way or another.

2b. I guess I think that the code is already ruined :) if you invoke this issue, the current behavior is to basically stop the world and never run any more callbacks on the Deferred (as it so happens; this isn't explicitly defined behavior). I think the exception will only help you to understand where your buggy code is going wrong.

2c. this one I'm really having a hard time interpreting, but I guess I can try to interpret it. The obvious way to replace a warning with an exception is to replace the "warnings.warn" line with a "raise" line... so wouldn't that cause the new exception to be raised in the same manner that -Werror would raise it?

2d. maybe. as I explained in #2b, though, the state of the Deferred is already screwed by invoking this behavior.

Given that I still don't understand all of your comments, I'll wait until you have a chance to read this before I merge.

comment:6 Changed 17 months ago by radix

  • Owner changed from radix to exarkun

comment:7 Changed 17 months ago by tom.prince

For 2c, my first thought would be that the exception should be handed to the errback chain. There actually appears to be two places where this would make sense. Either as if a Failure was returned, instead of the circular deferred, or instead of the None result that returning a deferred normally produces. I think that definitely the first should occur. I'm unsure about the second bit.

Perhaps something like the following would make the -Werror case behave like the eventual exception:

try:
  warnings.warn("...", category=DeprecationWarning)
except DeprecationWarning:
  current.result = Failure()

comment:8 Changed 17 months ago by exarkun

  • Owner changed from exarkun to tom.prince

First, thanks for your careful consideration of my meandering review comment.

2a. I'm not really sure where else it could be raised, so I guess it doesn't seem surprising to me.

As tom pointed out, the error could be reported down the errback chain. I think avoiding raising an exception out of _runCallbacks is actually quite important. Introducing a way for Deferred.callback to fail because of non-local behavior makes all Deferred-using code much more difficult to reason about (or, the reasoning is simple: all such code is probably wrong and buggy, because it doesn't handle exceptions from Deferred.callback ;)

Passing the exception down the errback chain seems quite attractive, since it forces the consumer of the Deferred to deal with the problem - and it seems to me that it is exactly the consumer of the Deferred that is likely to blame for this problem: that is the party responsible for an addCallback that introduced this circularity, right?

As far as implementing this goes, I'm tempted to suggest that the new check and warning belong *inside* the try/except where the callback function itself is called. I haven't considered this too deeply though, and I can vaguely see that there might be some problems with this. It does have the advantage of letting you not re-implement the Failure creation (which is nice, keeping in mind that proper Failure creation here is no longer just Failure() - yay captureVars).

2b. I guess I think that the code is already ruined :) if you invoke this issue, the current behavior is to basically stop the world and never run any more callbacks on the Deferred (as it so happens; this isn't explicitly defined behavior). I think the exception will only help you to understand where your buggy code is going wrong.

The warning/exception is great, and I'm all for it. I just want to be sure it doesn't introduce even *more* brokenness into programs that run afoul of this issue by introducing an exception into the Deferred creator/invoker codepath where previously there could not be one. A mysteriously hung Deferred that prevents a consumer from proceeding is bad and - indeed - already ruined, but worse is that case plus corrupted internal state in the creator/invoker because a new exception prevented half the implementation from running.

2c. this one I'm really having a hard time interpreting, but I guess I can try to interpret it. The obvious way to replace a warning with an exception is to replace the "warnings.warn" line with a "raise" line... so wouldn't that cause the new exception to be raised in the same manner that -Werror would raise it?

Sorry for the lack of clarity. Yes, a raise in the same place as the warn call would give us the same exception-y behavior. I wasn't sure if you thought the raise would go in the same place as the warn call, or if you'd already considered the potential issues of _runCallbacks starting to raise exceptions and thought that when this warning became a real exception the raise statement might go somewhere else. From discussion of the earlier review points, it sounds like the intent was for the raise to go where the warn call is, and so this review comment isn't very interesting. If we can agree that the exception should be reported down the errback chain, then I think it's a pretty obvious consequence that the warn call will be moved or otherwise adjusted so that when -Werror is in use that's the behavior we actually get.

comment:9 Changed 17 months ago by radix

  • Keywords review added
  • Owner tom.prince deleted

Okay, can someone take a look again?

comment:10 Changed 17 months ago by radix

sorry, I didn't specify the review points I dealt with and how I dealt with them.

Basically, I moved the warn() call into the try:except: that already exists around a callback, as exarkun suggested. I didn't notice any strange behavior with doing it this way; it emulates pretty well an exception simply being raised from the callback, which is basically the intent (there's a bug in your callback).

comment:11 Changed 17 months ago by radix

  • Keywords review removed
  • Resolution set to duplicate
  • Status changed from new to closed

Since #6164 was reopened due to a Python 3 build failure, and this branch actually resolves that error, I'm closing this ticket to continue work on the original branch.

Note: See TracTickets for help on using tickets.