Opened 7 years ago

Closed 7 years ago

#2729 task closed fixed (fixed)

remove Deferred.setTimeout usage from twisted.mail

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: mail Keywords:
Cc: exarkun, peaker, therve Branch:
Author: Launchpad Bug:

Description


Change History (13)

comment:1 Changed 7 years ago by exarkun

This is started in process-alias-timeout-2729. I think the tests still need a bit of work, though.

comment:2 Changed 7 years ago by Peaker

  • Cc exarkun added

The branch needs to be forward-merged with trunk or restarted.

Can't a similar fix as in http://twistedmatrix.com/trac/attachment/ticket/2245/cdefer.3.patch be used? (The patch includes more things, take a look just at the alias.py fix).

comment:3 Changed 7 years ago by Peaker

  • Cc peaker added

comment:4 Changed 7 years ago by therve

This not as simple, because your patch doesn't add tests.

comment:5 Changed 7 years ago by exarkun

  • Priority changed from normal to high

I merged this forward to process-alias-timeout-2729-2. One specific area I notice is short on test coverage is ProcessAliasProtocol.processEnded. If you run trial --coverage twisted.mail.test, you see that the entire else suite in that method is uncovered. The other area of the code I'm worried about is MessageWrapper._processEnded: the test suite only exercises it once - which means it only goes through the true case of the if self._timeoutCallID is not None: test, which means the conditional doesn't need to be there at all (as far as the test suite is concerned). A test which went through this method with that conditional evaluating to false would be nice - if it is possible to do that, otherwise the check should be dropped.

I'll get to this after I finish all the reviews, but if someone beats me to it that'd be great.

comment:6 Changed 7 years ago by exarkun

Oh yea, this is blocking #2245 which I want to review, so I guess I'll end up working on this first thing tomorrow.

comment:7 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Done in process-alias-timeout-2729-2

comment:8 Changed 7 years ago by exarkun

  • Priority changed from high to highest

comment:9 Changed 7 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to exarkun
  • unused import error in alias.py
  • StubProcess doesn't document its methods

Otherwise, that looks great, so please merge once done.

comment:10 Changed 7 years ago by exarkun

(In [21622]) remove unused import

refs #2729

comment:11 Changed 7 years ago by exarkun

(In [21624]) docstrings for StubProcess

refs #2729

comment:12 Changed 7 years ago by exarkun

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

(In [21628]) Merge process-alias-timeout-2729-2

Author: exarkun
Reviewer: therve
Fixes #2729

Improve test coverage for the aliases(5) process support, change the implementation
not to use Deferred.setTimeout, and fix some other cases related to process termination
which were handled incorrectly.

comment:13 Changed 3 years ago by <automation>

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