Opened 11 years ago

Closed 7 years ago

#1702 task closed fixed (fixed)

Kill Defer.setTimeout

Reported by: Jonathan Lange Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, Jean-Paul Calderone, Thijs Triemstra Branch: branches/settimeout-1702-4
branch-diff, diff-cov, branch-cov, buildbot
Author: therve


It's been deprecated for long enough to kill. People are starting to ask questions.

spiv, I'll review it if you do it :)

Attachments (1)

timeout_1702.diff (3.8 KB) - added by therve 10 years ago.

Download all attachments as: .zip

Change History (18)

Changed 10 years ago by therve

Attachment: timeout_1702.diff added

comment:1 Changed 10 years ago by therve

An easy one. There seems to be a few remaining call through twisted, though:

  • spread/ui/
  • spread/ui/
  • names/
  • mail/

I'll try to see if they can be removed easily.

comment:2 Changed 10 years ago by therve

Cc: therve added

comment:3 Changed 10 years ago by therve

I've removed spread and mail calls, and I've created #2414 for dns.

comment:4 Changed 10 years ago by therve

Owner: changed from spiv to therve

comment:5 Changed 10 years ago by spiv

What's holding this up? I've looked at the patch, and it looks good to me. Assuming tests pass, I'm happy for this to get merged.

comment:6 Changed 10 years ago by therve

Keywords: review added
Owner: changed from therve to spiv
Priority: normalhighest

That looks ready. Please review settimeout-1702-2.

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

Keywords: review removed
Owner: changed from spiv to therve

So, hmm.

A couple problems jump out at me.

  • there's no test coverage for twisted/spread/ui/ at all. Going strictly by the review process document, that's sufficient reason to not merge this as-is. Since it's UI code, though...
  • however, the code was also broken previously, and is broken in the same way after this change. if the brokenness were not innately tied to the use of setTimeout, this might not be a good reason to not merge the branch. But it is. ;P When the PB call eventually does complete, either successfully or with an error (eg ConnectionLost), it will blow up with an AlreadyCalledError. To avoid this, it is necessary to add a layer of indirection through a second Deferred - similar to the way ThreadedResolver works.

The latter point is basically the same problem as exists in, which the branch actually does address, albeit in a somewhat inelegant manner (catching the AlreadyCalledError) ;)

So I'm not sure if I'm okaying this branch or rejecting it, really. The mail changes certainly look correct and beneficial, maybe they should go in separately from the rest of this. Someone who worked on twisted/spread/ui/ should probably look at those changes and maybe try to write some tests somehow.

comment:8 Changed 10 years ago by therve

Owner: changed from therve to spiv
Priority: highestnormal

I understand you concerns, but there is basically no chance that I ever write test code for this ui stuff (which should probably be rewrite to be actually testable).

I can probably live with that for now. If you create a ticket for the mail issue I'd be happy to extract the change.

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

Cc: Jean-Paul Calderone added

The mail issue got created at some point by someone and the mail parts of this taken care of. The branch should be merged forward and the twisted/mail/ parts of it dropped (they conflict badly with what's in trunk now and are redundant).

As for the rest... The gtkutil change looks right, though it is untested. We should probably start thinking about dropping our gtk1 code anyway. However, the gtk2util change isn't as good as the gtkutil change and testing reveals that it indeed has a problem, 30 seconds after the connection is established, it logs an AlreadyCalledError traceback. If it did the same thing gtkutil did I think it would work.

comment:10 Changed 8 years ago by rotund

Author: rotund
Branch: branches/remove-Defer-setTimeout-1702

(In [26606]) Branching to 'remove-Defer-setTimeout-1702'

comment:11 Changed 8 years ago by rotund

Branch: branches/remove-Defer-setTimeout-1702branches/settimeout-1702-2

Modifying original branch

comment:12 Changed 8 years ago by rotund

Branch: branches/settimeout-1702-2branches/settimeout-1702-3

(In [26607]) Branching to 'settimeout-1702-3'

comment:13 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

#4150 was a duplicate of this

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

Author: rotundrotund, exarkun
Branch: branches/settimeout-1702-3branches/settimeout-1702-4

(In [29455]) Branching to 'settimeout-1702-4'

comment:15 Changed 7 years ago by Jean-Paul Calderone

Author: rotund, exarkuntherve
Owner: changed from spiv to therve

I recovered the code from the deleted settimeout-1702-2 branch. There were a bunch of conflicts, fortunately all resolved by picking the trunk version.

The branch needs a news fragment. Manual testing of manhole shows that the AlreadyCalledError traceback I last reported has been fixed (does someone remember doing that?) so I think this should be merged now, missing tests notwithstanding (and we all know that we shouldn't make a habit of that).

comment:16 Changed 7 years ago by therve

Resolution: fixed
Status: newclosed

(In [29496]) Merge settimeout-1702-4

Authors: therve, rotund Reviewer: exarkun Fixes: #1702

Remove the long deprecated Deferred.setTimeout method.

A new era of peace has started.

comment:17 Changed 6 years ago by <automation>

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