RFC821 upon which smtplib was originally based does not define a 421
status code and implies the server should only disconnect in response to
a QUIT command.

Subsequent extensions in RFC2821 (and now RFC5321) define situations
under which the server may return a 421 status and disconnect. This
leads to the following problem.

An smtplib.SMTP() instance is created and its sendmail() method is
called with a list of recipients which contains several invalid, local
addresses. sendmail() processes the recipient list, calling the rcpt()
method for each. Some of these may be accepted with a 250 or 251 status
and some may be rejected with a 550 or other status. The rejects are
kept in a dictionary to be eventually returned as the sendmail() result.

However, with the Postfix server at least, after 20 rejects, the server
sends a 421 Too many errors reply and disconnects, but sendmail
continues to process and this results in raising
SMTPServerDisconnected("Connection unexpectedly closed") and the
response dictionary containing the invalid addresses and their responses
is lost.

The caller may see the exception as retryable and may retry the send
after some delay, but since the caller has received no information about
the invalid addresses, it sends the same recipient list and the scenario
repeats.

So, if I understand the issue and the RFC correctly, when receiving a
421, we should close the connection and, if the 421 was received in the
"rcpt" phase, give back a dictionary with all pending directions as
"421" code, so the sending code knows they are 4xx codes and should
retry again later.

What return code would be give back if we get 421 in other situations?.
Maybe a regular "connection close"?.

I'm not completely sure about this, but here's my thoughts. In the
scenarios I've seen, the 421 reply/disconnect only occurs in response to
a RCPT which has an invalid address and follows several prior refused
RCPTs. In this case, I think the proper action is to close the
connection and raise SMTPRecipientsRefused and return a dictionary with
the actual responses for the refused RCPTS prior to the 421 and the 421
response only for the RCPT that produced it.

If the 421 comes at another time, I think the current process does the
right thing. It will raise the appropriate exception if it gets the
chance. It just needs to be sure that if the response was 421 that
instead of doing self.rset() it does self.close().

I have attached a patch against the 2.6.1 smtplib.py which I think does
the right thing. I haven't tested this at all, but I think it should
work. The documentation may need to be updated to emphasize that even
though all recipients aren't listed in the dictionary returned with the
SMTPRecipientsRefused exception, no one got the mail.

Smptlib is documented as RFC821/1869 based. So this is a request for a new feature. I presume more would be required to bring it up to 2821/5321. Assuming so, what is the rationale for adding just one new feature. This would mean tacking something like "and RFC 2821 for 421 status" on the end of "For details of SMTP and ESMTP operation, consult RFC 821 (Simple Mail Transfer Protocol) and RFC 1869 (SMTP Service Extensions)."

Can a test be written for this? There seem to be some coded status tests already.

I

The latest relevant RFC is 5321:

http://www.faqs.org/rfcs/rfc5321.html

smtplib should be reviewed for compliance with this updated spec.

Here is a patch along with test case update which can be cleanly applied into default.

I think this should be considered a bug fix.  While it is true that we are handling something that is a "more advanced" feature of the smpt protocl than what we are (currently) formally supporting in the module, the reality is that if one uses smtplib to communicate with a foreign mail server, you currently get unhandleable errors even if the library sticks to the currently supported RFC level.  I think that the fact that smtplib does the wrong then when it receives an unexpected error code followed by a connection close is a bug in smtplib regardless of what RFC level we are talking about.

In fact, this is one instance of a larger problem.  I have also seen a server that returns a 451 error code at the end of a data command *and then disconnects*.  I think that is out of spec even with the newer RFCs, but it is something that happens in the real world and so we should handle it.

The general problem in smtplib is that it does an rset on error, and if the other end has closed the connection, the rset raises a ServerDisconnectedError, masking the error code that the server actually reported.  This also should be fixed.

Clearly for 421 we should be doing a close and not an rset, and I think we may as well go ahead and do that even if we haven't upgraded the library to fully comply with the newer RFC yet, especially since we've got a patch here.  It is not a publicly visible API change, so I don't think the "new feature" rule needs to be applied strictly.  Correctly handling the case of rset finding the socket already closed would also have fixed this bug, and we should make that fix as well, since it seems that servers may unexpectedly close the connection after any error.

Given that David is usually more conservative about calling things bugs than I am, knows more about this subject than me, and has explained that the change will make smptlib act more sanely in the face of external errors, I am reversing myself and agreeing with him.

The patch seems to tighten the test in a way relevant to the code change.

I see that there are several open smptlib issues.
Should we open one for 'update to rfc 5321'?
Does the absence of 'obsoleted by' (or similar) mean that it is still the latest?

Sure, a new issue for making sure smtplib conforms to 5321 would be good (we already did it for smptd; hopefully we didn't miss anything).

For the moment 5321 is the standard.  There's a new one coming, where utf-8 will be allowed, but we're not quite ready to tackle that yet (soon, though, hopefully).

I had to rewrite the tests for 3.2, and I added tests to test all three cases where 421 is returned.  I'm in the process of merging this, but I'll upload the patch so that Kushal can see the test structure for the followon patch.

New changeset bc538bc8e65d by R David Murray in branch '3.2':
#5713: Handle 421 error codes during sendmail by closing the socket.
http://hg.python.org/cpython/rev/bc538bc8e65d

New changeset cf5c40025af2 by R David Murray in branch '3.3':
Merge: #5713: Handle 421 error codes during sendmail by closing the socket.
http://hg.python.org/cpython/rev/cf5c40025af2

New changeset ce4154268c47 by R David Murray in branch 'default':
Merge: #5713: Handle 421 error codes during sendmail by closing the socket.
http://hg.python.org/cpython/rev/ce4154268c47

New changeset fbf54209de75 by R David Murray in branch '3.2':
#5713: fix timing issue in smtplib tests.
http://hg.python.org/cpython/rev/fbf54209de75

New changeset 0de74602692f by R David Murray in branch '3.3':
Merge: #5713: fix timing issue in smtplib tests.
http://hg.python.org/cpython/rev/0de74602692f

New changeset 6dc948e4ea8f by R David Murray in branch 'default':
Merge: #5713: fix timing issue in smtplib tests.
http://hg.python.org/cpython/rev/6dc948e4ea8f

Thanks Mark and Kushal.

New changeset d068fcbe5009 by R David Murray in branch '3.3':
#5713: One more test_smtplib timing fix.
http://hg.python.org/cpython/rev/d068fcbe5009

New changeset fcef6a33de17 by R David Murray in branch 'default':
Merge: #5713: One more test_smtplib timing fix.
http://hg.python.org/cpython/rev/fcef6a33de17