Opened 9 years ago

Closed 9 years ago

#3638 defect closed fixed (fixed)

SMTPSenderFactory always fails on retry

Reported by: atsuoi Owned by:
Priority: normal Milestone:
Component: mail Keywords:
Cc: Jean-Paul Calderone, Thijs Triemstra Branch: branches/smtpsender-retry-3638-2
branch-diff, diff-cov, branch-cov, buildbot
Author: atsuoi, exarkun

Description

If SMTP session was failed after file transfer started, then retrying will always fail with following error:

Failure instance: Traceback (failure with no frames): <class 'twisted.mail.smtp.SMTPDeliveryError'>: No recipients accepted

I attached a patch to fix problem. With this patch, the file object passed to SMTPSenderFactory should have seek() method.

Attachments (2)

smtp.diff (706 bytes) - added by atsuoi 9 years ago.
test_smtp.diff (1.8 KB) - added by atsuoi 9 years ago.

Download all attachments as: .zip

Change History (16)

Changed 9 years ago by atsuoi

Attachment: smtp.diff added

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

Cc: Jean-Paul Calderone added
Owner: changed from Jean-Paul Calderone to ascii

Thanks for the patch. Can you also attach a unit test for the behavior being fixed?

comment:2 Changed 9 years ago by Glyph

Owner: changed from ascii to atsuoi

Fixing (apparent?) mis-assignment.

Changed 9 years ago by atsuoi

Attachment: test_smtp.diff added

comment:3 in reply to:  2 Changed 9 years ago by atsuoi

Add an unit test. I'm not familiar with Twisted's unit test, so the test may contain uncessecary codes. I'd appreciate to know how I can improve this test.

comment:4 Changed 9 years ago by atsuoi

Keywords: review added
Owner: atsuoi deleted

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
  1. Generally, tests shouldn't do things which require superuser privileges. Binding to port 25 will fail if the test suite is run as a normal user, which is almost always how it's run. Even if the suite is run as a superuser, it will fail if there's a real mail server listening. reactor.listenTCP(0, ...) will request that an unused port is used. The returned port object will tell you what port number was actually bound - port.getHost().port. This is generally the preferred thing to do if it's necessary to bind a port in a unit test.
  2. All classes and methods should have a docstring.
  3. In the Twisted naming convention, variables and attributes use the camelCase convention.

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

Author: exarkun
Branch: branches/smtpsender-retry-3638

(In [26174]) Branching to 'smtpsender-retry-3638'

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

Author: exarkunatsuoi, exarkun
Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

In addition to fixing the above points, I've also changed the test to run against a real SMTP server. I was tempted to leave the fake SMTP server in place, but the complexity of the interaction, particular the timing of certain events, tipped me over. It's a bit more code, but not much, and it should be a better test.

Build Results

comment:9 Changed 9 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to Jean-Paul Calderone

Make sure to update the copyright headers for 2009 in the files you modified.

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

Keywords: review added
Owner: Jean-Paul Calderone deleted

This branch touches the same files as #3641, which updates the copyright date. No point changing them here too.

comment:11 Changed 9 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Cool, +1.

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

Branch: branches/smtpsender-retry-3638branches/smtpsender-retry-3638-2

(In [26190]) Branching to 'smtpsender-retry-3638-2'

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

Resolution: fixed
Status: newclosed

(In [26193]) Merge smtpsender-retry-3638-2

Author: atsuoi, exarkun Reviewer: therve Fixes: #3638

Fix the retry support in SMTPSenderFactory so that if one or more recipient addresses is accepted or if part of the message body file is read and sent to the remote MTA that data is still used on subsequent delivery attempts. The previous behavior could exhaust the recipient address iterator on the first failed attempt or move the position pointer in the message body file causing subsequent deliver attempts to operate using invalid data.

comment:14 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.