Opened 5 years ago

Closed 4 years ago

#6572 enhancement closed fixed (fixed)

twisted.mail.smtp.sendmail should return a cancellable Deferred

Reported by: Kai Zhang Owned by: Kai Zhang
Priority: normal Milestone:
Component: mail Keywords:
Cc: ashfall Branch: branches/sendmail-deferred-cancellation-6572
branch-diff, diff-cov, branch-cov, buildbot
Author: kaizhang

Description

When a user call the cancel method of the Deferred returned by twisted.mail.smtp.sendmail, it should tell the twisted.mail.smtp.SMTPSenderFactory not to retry and disconnect the connection immediately.

Attachments (3)

6572-0.patch (5.2 KB) - added by Kai Zhang 5 years ago.
Add cancellation support to twisted.mail.smtp.sendmail. Add test for it.
6572-1.patch (3.0 KB) - added by Kai Zhang 5 years ago.
Add cancellation support to twisted.mail.smtp.sendmail. Write test code with twisted.test.proto_helpers.MemoryReactor.
6572-2.patch (6.2 KB) - added by Kai Zhang 4 years ago.
Revise the patch according to the comments.

Download all attachments as: .zip

Change History (29)

Changed 5 years ago by Kai Zhang

Attachment: 6572-0.patch added

Add cancellation support to twisted.mail.smtp.sendmail. Add test for it.

comment:1 Changed 5 years ago by Kai Zhang

I added the cancellation support to twisted.mail.smtp.sendmail. But there is a known issue with the test code. If the smtphost passed to twisted.mail.smtp.sendmail is a name instead of IP address. The test will raises the DirtyReactorAggregateError, which suggests the DelayedCall - ThreadedResolver._cleanup - is neither called nor cancelled. I thought the connector would take care of this when disconnect. So I don't know if this is a bug. Please tell me if I did something wrong.

Changed 5 years ago by Kai Zhang

Attachment: 6572-1.patch added

Add cancellation support to twisted.mail.smtp.sendmail. Write test code with twisted.test.proto_helpers.MemoryReactor.

comment:2 Changed 5 years ago by Kai Zhang

Keywords: review added

comment:3 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Kai Zhang

Thanks for your work on this!

  1. In the case where the connection to the server has already been made, you should be using abortConnection() to close it, but connector.disconnect() will use loseConnection().
  2. The assertion about factory.sendFinished seems unnecessary.
  3. You should probably have an assertion that the default _reactor argument is the global reactor (you can use inspect.getcallargs to do that).
  4. The docstring for sendmail should be updated to document that the Deferred returned is cancellable, and explain what cancelling does.
  5. The docstring for the test should say C{Deferred} or maybe L{defer.Deferred} rather than deferred.

comment:4 Changed 5 years ago by ashfall

Cc: ashfall added

Changed 4 years ago by Kai Zhang

Attachment: 6572-2.patch added

Revise the patch according to the comments.

comment:5 in reply to:  3 Changed 4 years ago by Kai Zhang

Replying to itamar:

Thanks for your work on this!

  1. In the case where the connection to the server has already been made, you should be using abortConnection() to close it, but connector.disconnect() will use loseConnection().
  2. The assertion about factory.sendFinished seems unnecessary.
  3. You should probably have an assertion that the default _reactor argument is the global reactor (you can use inspect.getcallargs to do that).
  4. The docstring for sendmail should be updated to document that the Deferred returned is cancellable, and explain what cancelling does.
  5. The docstring for the test should say C{Deferred} or maybe L{defer.Deferred} rather than deferred.

I have revised my patch according to the comments. There are two things worth noting:

  1. I use inspect.getargspec to assert that the default _reactor argument is the global reactor. Because inspect.getcallargs is new in version 2.7 and inspect.getargspec is not.
  2. Since twisted.test.proto_helpers.StringTransport doesn't have abortConnection method, I copied a version of StringTransport that supports abortConnection from twisted.web.test.test_newclient. Maybe we should add abortConection to twisted.test.proto_helpers.StringTransport later?

comment:6 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted

comment:7 Changed 4 years ago by ashfall

Author: ashfall
Branch: branches/sendmail-deferred-cancellation-6572

(In [38860]) Branching to 'sendmail-deferred-cancellation-6572'

comment:8 Changed 4 years ago by ashfall

Owner: set to ashfall
Status: newassigned

comment:9 Changed 4 years ago by Itamar Turner-Trauring

Rather than copying the StringTransport class, I suggest just importing it from the other test module.

comment:10 Changed 4 years ago by ashfall

Keywords: review removed
Owner: changed from ashfall to Kai Zhang
Status: assignednew

Thanks for working on this!

  1. twisted/mail/test/test_smtp.py:
  1. test methods don't have to document their return type.
  2. test_cancelBeforeConnectionMade returns a Deferred. But you only need to do that if you need the test to run the global reactor. Since you're using MemoryReactor that is unnecessary. But it also means you should use self.failureResultOf(d) to extract the Deferred and assert its result immediately, rather than relying on assertFailure (see the trial howto).
  3. Same for test_cancelAfterConnectionMade.
  4. Missing whitespace on line line 1713.
  5. Please use L{defer.Deferred} instead of C{Deferred}
  6. Is the line connector = reactor.connectors[0] in test_cancelAfterConnectionMade necessary?
  1. twisted/mail/smtp.py
  1. Replace variable "argh" with "result" ("argh" is an exclamation of distress or annoyance).
  2. "p" is a bad name for an attribute. "currentProtocol" might be better, or come up with some other more informative name.
  3. The line del self.p in removeProtocol is unnecessary.
  1. Missing news file.

comment:11 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted

Build Results

I revised the patch according to the comments of itamar and ashfall. The pyflakes complains about "redefinition of unused 'StringIO' from line 44" in twisted.mail.test.test_smtp. I think it's a misreporting.

comment:12 Changed 4 years ago by Kai Zhang

Keywords: review removed
Owner: set to Kai Zhang

comment:13 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted

The private argument _reactor of twisted.mail.smtp.sendmail is changed to public argument reactor.

Build Results

comment:14 Changed 4 years ago by Thijs Triemstra

Author: ashfallkaizhang

twisted/mail/test/test_smtp.py imports StringTransport from twisted.web.test.test_newclient, making twisted.mail (its tests) dependant on twisted.web, which doesn't seem right. Why did you change the original import?

comment:15 in reply to:  14 Changed 4 years ago by Kai Zhang

Replying to thijs:

twisted/mail/test/test_smtp.py imports StringTransport from twisted.web.test.test_newclient, making twisted.mail (its tests) dependant on twisted.web, which doesn't seem right. Why did you change the original import?

To test the cancellation, we need a StringTransport that support abortConnection. The original one doesn't support that. Itarmar and I discussed it and decided that we should file a ticket to add the abortConnection to the original StringTransport. Before that, Itarmar suggested just importing it from the other test module. You can see his comment in reply 9.

comment:16 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Kai Zhang

Thanks for all your work!

  1. Switch back to copy/pasting AbortableStringTransport, but add a comment saying "This should be replaced by a common version in #6530." Not good, but yeah, we shouldn't have unnecessary cross-package dependencies.
  2. The docstring for sendmail() should say "it will stop retry" rather than "it will stop retrying".
  3. I think there's a bug in the logic for deciding what to do in cancellation. Consider the following scenario: connection succeeds, there's some problem, connection disconnects, new attempt at connection is made, operation is cancelled. The canceller will see that currentProtocol is set, but it's still the old protocol, and it will call transport.abortConnection() on it rather than stopping the new connection attempt. You should write a test for this edge case, and then once you've reproduced the problem fix the bug, or if you discover it's not an actual bug add a comment on the ticket explaining your reasoning.

Please fix the above and resubmit.

comment:17 in reply to:  16 ; Changed 4 years ago by Kai Zhang

Status: newassigned

Replying to itamar:

  1. The docstring for sendmail() should say "it will stop retry" rather than "it will stop retrying".

Hi, the docstring for sendmail() does say "it will stop retry" now. Should I change it to "it will stop retrying"?

comment:18 in reply to:  17 Changed 4 years ago by ashfall

Replying to kaizhang:

Replying to itamar:

  1. The docstring for sendmail() should say "it will stop retry" rather than "it will stop retrying".

Hi, the docstring for sendmail() does say "it will stop retry" now. Should I change it to "it will stop retrying"?

Yes please.

comment:19 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted
Status: assignednew

comment:20 in reply to:  16 Changed 4 years ago by Kai Zhang

Replying to itamar:

  1. I think there's a bug in the logic for deciding what to do in cancellation. Consider the following scenario: connection succeeds, there's some problem, connection disconnects, new attempt at connection is made, operation is cancelled. The canceller will see that currentProtocol is set, but it's still the old protocol, and it will call transport.abortConnection() on it rather than stopping the new connection attempt. You should write a test for this edge case, and then once you've reproduced the problem fix the bug, or if you discover it's not an actual bug add a comment on the ticket explaining your reasoning.

I have reproduced the problem. I think it's a bug of SMTPSenderFactory processing client connection error after we added the currentProtocol attribute rather than a bug of cancellation. So I added tests for removing the current protocol when client connection is failed/lost and fixed the bug.

Also, since around half supported builders have been shutdown. I didn't force a build upon the new patch. I will add the build results once the baelnorn issue is addressed.

comment:21 Changed 4 years ago by Tom Prince

Looking at this code, I wondered if it might ever be the case that .buildProtocol has been calledc, but not .makeConnection. Looking at the code for TCP connections, it looks like those are always called synchronously, so that there is no opportunity for .cancel to be called between them.

comment:22 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Kai Zhang

Thanks for your work on this.

  1. currentProtocol: Please add documentation for this in the class docstring.
  2. sendFinished:
    1. It appears that you are changing the meaning of this slightly. Please add documentation for this.
      • Originally, it being 1 (or True, this code probably predates True) meant that the message had been sent, or there has been an unrecoverable error.
      • Now, it may also mean that the sending should be cancelled.
    2. Also, you should set it to True instead of 1 in cancel.

Please resubmit after making the above changes.

comment:23 in reply to:  22 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted

I have revised my patch according to the comments.

Build Results

Replying to tom.prince:

  1. Also, you should set it to True instead of 1 in cancel.

The original implementation will compare sendFinished with 0 and retries(see twisted.mail.smtp.SMTPSenderFactory._processConnectionError), so I think it'd be better to set sendFinished to 1 in cancel.

comment:24 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Kai Zhang
  1. Also, you should set it to True instead of 1 in cancel.

The original implementation will compare sendFinished with 0 and retries(see twisted.mail.smtp.SMTPSenderFactory._processConnectionError), so I think it'd be better to set sendFinished to 1 in cancel.

I understand your desire to match the existing code, but in this case the existing code isn't an example of a style to follow.

The values you assign to sendFinished should be True/False and the documented type bool.

Old versions (before 2.2) of python didn't have True and False, and there is lots of code (like the code you are modifying it) that predates this. They thus had to use 1 and 0 for those values, and apparently, the clever

if self.retries < self.sendFinished <= 0:

In new code, this would be better expressed as

#!
if (self.retries < 0) and (not self.sendFinished):

You shouldn't change that in this ticket but, you shouldn't avoid using True and False for sendFinished just because some old code was trying to be clever (and thus hard to understand). In fact, the code confused me somewhat, until I realized that self.retries is the negative of the number of remaining retries.

Thanks for working on this. Please merge after changing the documentation of sendFinished to refer to bool, True and False and *new* code to use those values.

comment:25 Changed 4 years ago by Kai Zhang

Status: newassigned

comment:26 Changed 4 years ago by Kai Zhang

Resolution: fixed
Status: assignedclosed

(In [39157]) Merge sendmail-deferred-cancellation-6572

Author: kaizhang Reviewer: tom.prince Fixes: #6572

Now twisted.mail.smtp.sendmail returns a cancellable Deferred. When a user cancels the Deferred, the twisted.mail.smtp.SMTPSenderFactory will stop retrying and disconnect the connection immediately.

Note: See TracTickets for help on using tickets.