A reentrant addCallbacks on a Deferred adds to mid-chain

[Peaker]

Specifically, addCallbacks that executes from within the context of a callback, adds the callback after the current executing callback, rather than at the end of the chain.

[Peaker]

Oops, the branch is: addcallbacks-2849

[jerub]

This code has tests, and the entire test suite runs without problems with the changes made. It worries me that this bug has slipped through.

I want to approve this commit, but I think that it's important to test some third party code just to make sure we're not going to break anyone else's codebase.

Perhaps run some of the Divmod test suite on this branch?

[therve]

I've run nevow test suite with success, so it looks good. My only complain would be to add documentation for _runningCallbacks. But the final approval should go to our chief surgeon :).

[exarkun]

Only looked at the code for a few seconds, but I noticed test_reentrantRunCallbacks doesn't return the Deferred it is playing with. It's not really clear what the ZeroDivisionError there is helping to test (it certainly isn't documented).

[Peaker]

Thanks for the review. I documented the reason behind the ZeroDivisionError and returned the deferred.

[exarkun]

(In [21523]) better docstrings; reflow comments to proper width; other minor changes

refs #2849

[exarkun]

(In [21524]) split the failure assertions into a separate test method

refs #2849

[exarkun]

(In [21525]) update comment to talk about the new implementation

refs #2849

[exarkun]

I did the stuff mentioned above. I think the branch looks fine now, although I noticed what might qualify as another bug while reviewing this. I filed #2861 for that.

[therve]

test_reentrantRunCallbacksWithFailure has an unused function callback2. In the same function, I think it would be more clear to have a separated function instead of a lambda followed by an assertEquals.

[exarkun]

(In [21527]) remove unused nested function; define another one to assert about the resulting exception

refs #2849

[therve]

Perfect, please merge!

[exarkun]

(In [21529]) Merge addcallbacks-2849

Author: peaker (mostly), exarkun Reviewer: jerub, therve, exarkun Fixes #2849

Fix Deferred.addCallback for the case where it is called on a Deferred from a callback on that same Deferred. The behavior is changed so that the new callback is added to the end of the callback chain and only called after the executing callback returns at the earliest.