Opened 15 months ago

Last modified 12 months ago

#6588 enhancement new

Commands of twisted.mail.pop3client.POP3Client should return a cancellable Deferred

Reported by: kaizhang Owned by: kaizhang
Priority: normal Milestone:
Component: mail Keywords:
Cc: ashfall Branch: branches/pop3client-deferred-cancellation-6588
(diff, github, buildbot, log)
Author: kaizhang Launchpad Bug:

Description

Commands of twisted.mail.pop3client.POP3Client should return a cancellable Deferred. If a command in blocked queue is cancelled, it should be removed from the queue. If a trying command is cancelled, the connection should be disconnected without sending the "QUIT" command, so that the operation wouldn't take affect on the server side.

Change History (16)

comment:1 Changed 15 months ago by kaizhang

  • Author set to kaizhang
  • Branch set to branches/pop3client-deferred-cancellation-6588

(In [38879]) Branching to 'pop3client-deferred-cancellation-6588'

comment:2 Changed 15 months ago by kaizhang

  • Keywords review added
  • Owner kaizhang deleted

Build Results

The freebsd-8.2-i386 buildbot seems to be broken.

comment:3 Changed 15 months ago by kaizhang

  • Keywords review removed
  • Owner set to kaizhang

comment:4 Changed 15 months ago by kaizhang

  • Keywords review added
  • Owner kaizhang deleted

comment:5 Changed 15 months ago by kaizhang

  • Keywords review removed
  • Owner set to kaizhang

comment:6 Changed 15 months ago by kaizhang

  • Status changed from new to assigned

comment:7 Changed 15 months ago by kaizhang

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

I rewrote the tests in a TDD development way.

Build Results

comment:8 follow-up: Changed 14 months ago by itamar

  • Cc ashfall added
  • Keywords review removed
  • Owner set to kaizhang

Thanks for reworking the tests.

  1. If you're overriding POP3Client for testing purposes, you don't need super detailed docstrings. So e.g. no need to explain stuff already explained in the base protocol class. This is test code, no one will use it in real code, goal is just to make it readable enough for people looking at the tests.
  2. The docstring for test_cancelCommandInQueueReturnedBySendShort talks about a case which I'm pretty sure shouldn't be handled by this test (everything after "The connection is NOT disconnected."). The second half of the test seems like it's already covered by other tests later on. Same for the sendLong equivalent test.
  3. "When cancel a command in the blocked queue" should be "when cancelling a command in the blocked queue".
  4. Also, "blocked queue" is a bit obscure - perhaps say "When cancelling a command that hasn't yet been sent over the network". In general test descriptions should be slightly higher level if possible, so you get a sense of overall intention rather than just implementation details.
  5. Using explicit assertions is usually a good idea in tests. So instead of:
    failure = self.failureResultOf(deferred)
    failure.trap(defer.CancelledError) 
    
    I'd instead do
    failure = self.failureResultOf(deferred)
    self.assertTrue(failure.check(defer.CancelledError))
    
  6. The dataReceived calls in the assertions about commands using sendShort/sendLong seem unnecessary.
  7. Might want assertIdentical in those assertions (not a big deal, since Deferreds don't actually implement custom comparison, but worth paying attention to the difference.)
  8. Add C{} around all the command names in those tests, e.g. "The C{stat} command uses etc.".

Please address and resubmit.

comment:9 Changed 14 months ago by kaizhang

  • Status changed from new to assigned

comment:10 in reply to: ↑ 8 Changed 14 months ago by kaizhang

I have revised my patch according to the comments.

Replying to itamar:

  1. The docstring for test_cancelCommandInQueueReturnedBySendShort talks about a case which I'm pretty sure shouldn't be handled by this test (everything after "The connection is NOT disconnected."). The second half of the test seems like it's already covered by other tests later on. Same for the sendLong equivalent test.

The second half of the test_cancelCommandInQueueReturnedBySendShort is the case where a command being popped out from the queue and sent over the network. What the cancellation do in this case is same to the case where the command is sent directly. But these two cases are trigger by different condition. So I splitted the test_cancelCommandInQueueReturnedBySendShort into two tests, keep alongside with the later test. Same for the sendLong equivalent test.

comment:11 Changed 14 months ago by kaizhang

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

comment:12 Changed 14 months ago by exarkun

Using explicit assertions is usually a good idea in tests. So instead of:

failure = self.failureResultOf(deferred)
failure.trap(defer.CancelledError) 

I'd instead do

failure = self.failureResultOf(deferred)
self.assertTrue(failure.check(defer.CancelledError))

This produces a slightly worse failure from the test:

exarkun@top:/tmp$ trial test_mode.py 
test_mode
  FailureModeTests
    test_check ...                                                       [FAIL]
    test_trap ...                                                       [ERROR]

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/tmp/test_mode.py", line 12, in test_check
    self.assertTrue(f.check(ValueError))
twisted.trial.unittest.FailTest: None

test_mode.FailureModeTests.test_check
===============================================================================
[ERROR]
Traceback (most recent call last):
Failure: exceptions.OSError: 

test_mode.FailureModeTests.test_trap
-------------------------------------------------------------------------------
Ran 2 tests in 0.011s

FAILED (failures=1, errors=1)

With Failure.check, you get no indication of what the Failure was. Of course, with trap you get no indication of where the problem occurred.

Fortunately, failureResultOf now takes extra arguments, producing better output:

    def test_failureResultOf(self):
        self.failureResultOf(fail(OSError()), ValueError)
[FAIL]
Traceback (most recent call last):
  File "/tmp/test_mode.py", line 17, in test_failureResultOf
    self.failureResultOf(fail(OSError()), ValueError)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/_synctest.py", line 674, in failureResultOf
    result[0].getTraceback()))
twisted.trial.unittest.FailTest: Failure of type (exceptions.ValueError) expected on <Deferred at 0x249e560 current result: None>, found type <type 'exceptions.OSError'> instead: Traceback (most recent call last):
Failure: exceptions.OSError: 


test_mode.FailureModeTests.test_failureResultOf

comment:13 Changed 13 months ago by tom.prince

  • Keywords review removed
  • Owner set to kaizhang

Thanks for working on this.

  1. As exarkun suggested, please use the additional argument to failureResultOf, rather than .assertTrue(failure.check(...)). (I filed #6721 to document this).
    • Sorry for the back-and-forth on this point. What you originally had is what all the existing code using failureResultOf in twisted does, and it is what is currently documented in the trial howto.
  2. There are a number of places in the test docstrings where you have The C{comething} command uses .... It appears that in most (all?) of the cases, you are refering to a method on POP3Client. You can refer to that more explicitly as The L{POP3Client.something} method uses ... or simply L{POP3Client.something} uses.
  3. "trying commnd" is awkward. "command being tried" is somewhat more comprehensible to a native speaker, but "current command" (or maybe "executing command) would be idiomatic in this case.
    • "and is trying to send the message": If the command has been poped out and the deferred is being cancelled then the command has been sent and is waiting for a response.
  4. MemoryPOP3Client: "memory" isn't a verb: you probably want either "that can remember the" or "that can record the" instead of "that can memory the".
  5. (minor) Perhaps "all the commands waiting in the queue" instead of "all the waiting commands in the queue". The later formulation causes me to want to ask about the non-waiting commands in the queue.
  6. The interaction between the interaction between chainDeferred, cancel and the two cancellers here is somewhat confusing. It took my some time to convince myself that calling one canceller from the other does the correct thing. And, in fact, _cancelTryingCommand gets called with a different deferred, dependingon whether it gets called directly, or from the other canceller.
    • I'm not sure what to suggest to make this clearer.
    • One option for addressing the last point, is to do self.waiting.cancel() instead of self._cancelTryingCommand in the cancel in _blocked, but I don't know if that is clearer.

Please resubmit for review after addressing 1-4 above.

comment:14 Changed 13 months ago by kaizhang

  • Status changed from new to assigned

comment:15 Changed 13 months ago by kaizhang

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

comment:16 Changed 12 months ago by tom.prince

  • Keywords review removed
  • Owner set to kaizhang

Thanks for working on this.

One minor point: Proper names (like method names) typicaly don't have an article (i.e "the"). "L{methodname}" rather than "The L{methodname}" (see here).

Please merge after changing that.

Note: See TracTickets for help on using tickets.