context management support in imaplib, smtplib, ftplib

[tarekziade]

BPO	4972
Nosy	@abalkin, @pitrou, @giampaolo, @tarekziade, @ezio-melotti, @merwok, @bitdancer, @berkerpeksag, @serhiy-storchaka
Dependencies	bpo-11289: smtplib context manager
Files	ftplib.diff imaplib.diff smtplib.diff ftplib.patch imaplib_with_2.patch imaplib_with_3.patch issue4972_imaplib.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2014-09-09.16:14:43.475>
created_at = <Date 2009-01-17.17:28:09.437>
labels = ['type-feature', 'library']
title = 'context management support in imaplib, smtplib, ftplib'
updated_at = <Date 2014-09-09.16:14:43.451>
user = 'https://github.com/tarekziade'

bugs.python.org fields:

activity = <Date 2014-09-09.16:14:43.451>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2014-09-09.16:14:43.475>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2009-01-17.17:28:09.437>
creator = 'tarek'
dependencies = ['11289']
files = ['12799', '12812', '12823', '17277', '28871', '29091', '35972']
hgrepos = []
issue_num = 4972
keywords = ['patch']
message_count = 31.0
messages = ['80026', '80158', '80202', '80203', '80210', '80280', '80314', '80321', '80348', '80351', '80352', '80393', '93914', '93915', '105293', '105403', '105408', '105409', '105410', '105411', '105414', '105415', '105440', '131015', '180730', '180761', '180785', '182244', '223228', '226640', '226642']
nosy_count = 11.0
nosy_names = ['belopolsky', 'pitrou', 'giampaolo.rodola', 'tarek', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'stuaxo', 'SilentGhost', 'berker.peksag', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue4972'
versions = ['Python 3.5']

[tarekziade]

In a program, I naturally wrote:

    >>> from ftplib import FTP
    >>> with FTP('ftp.somewhere.com') as ftp:
    ...     ftp.login('someone', 'pass')
    ...     ...

Until I realized this class is not equipped with __enter__ and __exit__,

I think it could be a simple and pleasant enhancement to add it.

[tarekziade]

positive feedbacks on python-ideas, so I'll start to write the patches.

targets :

• smtplib.SMTP
• imaplib.IMAP4
• ftplib.FTP

first patch : smtplib

(will do ftplib and imaplib as well, then propose this enhancement to
python-dev)

[abalkin]

What is the rationale for swallowing all socket exceptions except
"Connection reset by peer" in __exit__? In any case, it is better to use errno.ECONNRESET instead of literal 54.

Note that SMTP.quit() calls SMTP.close(), so in the normal termination
case, close will be called twice. This is not a real problem since SMTP.close() is a noop on a closed SMTP object, but it does not look
right.

The double call to close() also makes error path harder to analyze. It
appears that if a socket error is raised in the first call to close, it
gets caught only to be raised again in the second call (assuming a
persistent error).

[tarekziade]

What is the rationale for swallowing all socket exceptions except
"Connection reset by peer" in __exit__?

I am catching just the error that raises if the connection is closed
when calling quit()

In any case, it is better to use errno.ECONNRESET instead of literal 54.

Right

Note that SMTP.quit() calls SMTP.close(), so in the normal termination
case, close will be called twice. This is not a real problem since
SMTP.close() is a noop on a closed SMTP object, but it does not look
right.

Right i'll fix that

Thanks for teh feedback
The double call to close() also makes error path harder to analyze. It
appears that if a socket error is raised in the first call to close, it
gets caught only to be raised again in the second call (assuming a
persistent error).

[abalkin]

On Mon, Jan 19, 2009 at 2:01 PM, Tarek Ziadé <report@bugs.python.org> wrote:

Tarek Ziadé <ziade.tarek@gmail.com> added the comment:

> What is the rationale for swallowing all socket exceptions except
> "Connection reset by peer" in __exit__?

I am catching just the error that raises if the connection is closed
when calling quit()

I see. I misread the double negative "except errno NOT equals 54", but
I still don't see the rationale for that exception. I any case, I
don't think your patch implements that because SMTP transforms socket
errors into SMTPServer* errors in send():

        if self.sock:
            try:
                self.sock.sendall(str)
            except socket.error:
                self.close()
                raise SMTPServerDisconnected('Server not connected')
        else:
            raise SMTPServerDisconnected('please run connect() first')

so you will never see a socket error from quit().

Furthermore, I don't think you should ignore return code from quit():
you should raise an error if it returns anything but 221.

[tarekziade]

so you will never see a socket error from quit().

It did happen but on the close() call inside the quit() call.
I couldn't reproduce it in the tests yet, still working on it.

I have changed the patch and took car of the return code.

I have also commited a first version of the imaplib one. This one
required to write a dummy imap server in the tests, and I am not fully
happy with it yet (the returns are based on trials-errors, and I am not
fully understanding this protocol yet)

but imaplib is undertested, this fake server might be a good start for
more tests in test_imaplib imho.

[abalkin]

Please remove the old smtplib.patch: it is confusing to have two
attachments with the same name. It will still be available in the
history, so nothing will be lost.

A nit-pick: 221 is a success code (in smtp 2xx codes are successes and
5xx are errors IIRC), so vars should be simply 'code' (or 'rc' for
return code) and 'msg', not 'errcode' and 'errmsg'.

Your new code may leave socket open in the case when self.docmd("quit")
raises an exception other than SMTPServerDisconnected.

BTW, I think your code will be simpler if you don't reuse quit() and
simply call docmd("quit") and close()directly in an appropriate
try/except/finally statement.

[tarekziade]

I've simplified the code
accordinglyhttp://bugs.python.org/issue4972?@ok_message=issue%204972%20files%20edited%20ok&@template=item

about the name of the vars. while I agree with you, I have use those names
(errcode, errmsg) because they are used in getreply() so I thought it
was more homogeneous.

about the return code , i am wondering if it would be good to add in
ftplib all the constantes
(http://www.ftpplanet.com/ftpresources/ftp_codes.htm) so things are clearer

[tarekziade]

Feedback from my blog :

other places where a context manager could be good to have :

• gzip
• zipfile
• urllib2.urlopen and urllib.request.urlopen

[pitrou]

Tarek: gzip and bz2 are already done
(http://code.python.org/hg/trunk/rev/d9555936ded9).

[tarekziade]

Ah great, thanks for telling Antoine, I missed it on gzip. And the test
on bz2 makes me see that my tests are not sufficient (calling with on an
instance that has already been used and close)

[giampaolo]

As for ftplib patch you should make sure that close() gets called in any
case but never more than once.
You can use "finally" to satisfy the first requirement and "self.sock is
not None" to check the second condition:

+ def __exit__(self, *args):
+ """Context management protocol.
+
+ Will try to quit() if active, then close().
+ If not active, a simple close() call is made.
+ """
+ if self.sock is not None and self.getwelcome() is not None:
+ try:
+ self.quit()
+ except socket.error, e:
+ # [Errno 61] is Connection refused
+ # if the error is different, we want to raise it.
+ if e.errno != 61:
+ raise
+ finally:
+ if self.sock is not None:
+ self.close()
+ else:
+ self.close()

[stuaxo]

zipfile also would make a good target for a contextmanager (as noted here -
http://tarekziade.wordpress.com/2009/01/20/python-standard-lib-give-me-more-withs/
)

[ezio-melotti]

There's a patch for zipfile in bpo-5511.

[giampaolo]

Is this still desirable?
If so I can work on a patch for ftplib which provides a finer error handling in case of disconnection while sending QUIT and updates tests which should be modified to fit in the newer test suite.

[stuaxo]

It would be good for consistency, yes.

[giampaolo]

A patch including tests and doc changes is in attachment.

[merwok]

Magic methods usually don’t have docstrings, since they implement one operation that is described in one place (think __len__ vs. len). You could remove the doc of __enter__ and turn the doc of __exit__ into a comment or move it to the reST file when you say that FTP supports the context management protocol.

What did you add a cmd_noop method that does the same thing as cmd_user for?

Will the change from handler to handler_instance not break third-party code?

[giampaolo]

Magic methods usually don’t have docstrings, since they implement one
operation that is described in one place (think __len__ vs. len).

Agreed, I only left it since it was there in the first place.

What did you add a cmd_noop method that does the same thing as
cmd_user for?

Basically all DummyFTPHandler commands are no-ops except pasv, port, retr, stor and some others. There's no real reason why I added NOOP except for consistency with the FTP procol.
What I wanted to do in the new tests was just sending a command and receive a response and the "correct" way to do that in FTP is using NOOP.

Will the change from handler to handler_instance not break
third-party code?

Not sure what you mean by "third party code" but it shouldn't break the existing tests if that's what you're worried about.
I did that because the original server behavior was to accept only one connection and I needed to connect more than one client in the same test.

[merwok]

Re-reading the patch, I notice it’s in the test module, not on the FTP
class, so I guess it’s perfectly fine. Is the string returned by a real
FTP server “331 username ok”? (My point is that I think it would be
better to have different strings for cmd_noop and cmd_user.)

I meant code outside the standard library. I thought this was in ftplib,
but since it’s in test_ftplib we don’t have to fear breaking other
people’s code.

In summary, the diff for ftplib seems good to me, with a minor remark
about the docstrings, and I don’t feel confident enough to give a review
about the tests.

[giampaolo]

Is the string returned by a real FTP server “331 username ok”? (My
point is that I think it would be better to have different strings for
cmd_noop and cmd_user.)

Whoops! No that should have been "200 noop ok".
I copied cmd_user code and forgot to modify the response string.
Thanks.

New patch in attachment (I changed the doc a little bit including the "with" example usage in ftplib.rst other than just in what's new).

[merwok]

Ah, I’m glad I had one interesting comment out of three in my first
message :)

This change, although nice, seems a bit trivial for warranting taking so
much place in the what’s new. Or perhaps it’s a good thing; I don’t know
if people are aware of the goodness of the with statement far and wide.
I’m sure amk will edit the document if he thinks the example is out of
place.

[giampaolo]

This is now committed as r81041.
I've also removed the long description from what's new file, as you were suggesting.

The other two patches should be adapted for 3.2.

[SilentGhost]

The work on smtplib context manager continues in bpo-11289. I'm putting it as a dependency here.

[ezio-melotti]

ftplib.FTP and smtplib.SMTP support the "with" statement since 3.2 and 3.3 respectively.
imaplib seems to be the only one left.
Can someone review and commit the patch for 3.4?

[serhiy-storchaka]

Imaplib tests was changed a lot since 3.2. Here is an updated patch.

[berkerpeksag]

The patch lacks documentation updates.

[serhiy-storchaka]

Here is an updated patch. Updated documentation, added checks that logout executed after a with statement, now logout() can be called inside a with statement.

[berkerpeksag]

Updated Serhiy's patch to 3.5. I also added a whatsnew entry.

[serhiy-storchaka]

Thanks Berker.

[serhiy-storchaka]

I think that's all with this issue.