Opened 7 years ago

Closed 7 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: Launchpad Bug:

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 7 years ago by Peaker

  • Keywords review added
  • Owner glyph deleted

comment:2 Changed 7 years ago by Peaker

Oops, the branch is: addcallbacks-2849

comment:3 Changed 7 years ago by 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?

comment:4 Changed 7 years ago by therve

  • Owner set to exarkun
  • Priority changed from normal to highest

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 7 years ago by therve

  • Cc therve added

comment:6 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun 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 7 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 7 years ago by Peaker

  • Owner set to exarkun

comment:9 Changed 7 years ago by exarkun

  • Status changed from new to assigned

comment:10 Changed 7 years ago by exarkun

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

refs #2849

comment:11 Changed 7 years ago by exarkun

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

refs #2849

comment:12 Changed 7 years ago by exarkun

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

refs #2849

comment:13 Changed 7 years ago by exarkun

  • 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 7 years ago by exarkun

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

comment:15 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

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 7 years ago by exarkun

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

refs #2849

comment:17 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

comment:18 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Perfect, please merge!

comment:19 Changed 7 years ago by exarkun

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

(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 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.