Opened 10 years ago

Closed 10 years ago

#2849 defect closed fixed (fixed)

A reentrant addCallbacks on a Deferred adds to mid-chain

Reported by: Peaker Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch:
Author:

Description

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.

Change History (20)

comment:1 Changed 10 years ago by Peaker

Keywords: review added
Owner: Glyph deleted

comment:2 Changed 10 years ago by Peaker

Oops, the branch is: addcallbacks-2849

comment:3 Changed 10 years ago by Stephen Thorne

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?

comment:4 Changed 10 years ago by therve

Owner: set to Jean-Paul Calderone
Priority: normalhighest

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 :).

comment:5 Changed 10 years ago by therve

Cc: therve added

comment:6 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Peaker

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

comment:7 Changed 10 years ago by Peaker

Keywords: review added
Owner: Peaker deleted

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

comment:8 Changed 10 years ago by Peaker

Owner: set to Jean-Paul Calderone

comment:9 Changed 10 years ago by Jean-Paul Calderone

Status: newassigned

comment:10 Changed 10 years ago by Jean-Paul Calderone

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

refs #2849

comment:11 Changed 10 years ago by Jean-Paul Calderone

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

refs #2849

comment:12 Changed 10 years ago by Jean-Paul Calderone

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

refs #2849

comment:13 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed

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.

comment:14 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

comment:15 Changed 10 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

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.

comment:16 Changed 10 years ago by Jean-Paul Calderone

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

refs #2849

comment:17 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

comment:18 Changed 10 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Perfect, please merge!

comment:19 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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

comment:20 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.