classification
Title: context management support in imaplib, smtplib, ftplib
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.4
process
Status: open Resolution:
Dependencies: 11289 Superseder:
Assigned To: Nosy List: SilentGhost, belopolsky, berker.peksag, eric.araujo, ezio.melotti, giampaolo.rodola, pitrou, serhiy.storchaka, stuaxo, tarek
Priority: normal Keywords: patch

Created on 2009-01-17 17:28 by tarek, last changed 2013-02-16 22:14 by serhiy.storchaka.

Files
File name Uploaded Description Edit
ftplib.diff tarek, 2009-01-19 19:03
imaplib.diff tarek, 2009-01-20 21:08
smtplib.diff tarek, 2009-01-21 11:21
ftplib.patch giampaolo.rodola, 2010-05-09 19:50
imaplib_with_2.patch serhiy.storchaka, 2013-01-27 10:52 review
imaplib_with_3.patch serhiy.storchaka, 2013-02-16 22:14 review
Messages (28)
msg80026 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-17 17:28
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.
msg80158 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-19 10:43
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)
msg80202 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-19 18:46
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).
msg80203 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-19 19:01
> 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).
msg80210 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-19 21:21
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.
msg80280 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-20 21:07
> 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.
msg80314 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-21 02:27
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.
msg80321 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-21 11:20
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
msg80348 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-21 21:52
Feedback from my blog :

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

- gzip
- zipfile
- urllib2.urlopen and urllib.request.urlopen
msg80351 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-01-21 21:57
Tarek: gzip and bz2 are already done
(http://code.python.org/hg/trunk/rev/d9555936ded9).
msg80352 - (view) Author: Tarek Ziadé (tarek) * (Python committer) Date: 2009-01-21 22:11
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)
msg80393 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-01-23 01:44
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()
msg93914 - (view) Author: Stuart Axon (stuaxo) Date: 2009-10-13 01:57
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/
)
msg93915 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2009-10-13 02:04
There's a patch for zipfile in #5511.
msg105293 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-08 14:31
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.
msg105403 - (view) Author: Stuart Axon (stuaxo) Date: 2010-05-09 17:14
It would be good for consistency, yes.
msg105408 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-09 19:02
A patch including tests and doc changes is in attachment.
msg105409 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-05-09 19:10
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?
msg105410 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-09 19:25
> 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.
msg105411 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-05-09 19:35
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.
msg105414 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-09 19:50
> 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).
msg105415 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-05-09 19:57
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.
msg105440 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-10 15:00
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.
msg131015 - (view) Author: SilentGhost (SilentGhost) Date: 2011-03-15 18:33
The work on smtplib context manager continues in #11289. I'm putting it as a dependency here.
msg180730 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-01-27 01:31
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?
msg180761 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-27 10:52
Imaplib tests was changed a lot since 3.2. Here is an updated patch.
msg180785 - (view) Author: Berker Peksag (berker.peksag) * Date: 2013-01-27 19:42
The patch lacks documentation updates.
msg182244 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-16 22:14
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.
History
Date User Action Args
2013-02-16 22:14:00serhiy.storchakasetfiles: + imaplib_with_3.patch

messages: + msg182244
2013-01-27 19:42:42berker.peksagsetnosy: + berker.peksag

messages: + msg180785
title: context managerment support in imaplib, smtplib, ftplib -> context management support in imaplib, smtplib, ftplib
2013-01-27 10:52:50serhiy.storchakasetfiles: + imaplib_with_2.patch
nosy: + serhiy.storchaka
messages: + msg180761

2013-01-27 01:31:11ezio.melottisetstage: needs patch -> patch review
messages: + msg180730
versions: + Python 3.4, - Python 3.2, Python 3.3
2011-03-15 18:33:13SilentGhostsetversions: + Python 3.3
nosy: + SilentGhost

messages: + msg131015

dependencies: + smtplib context manager
2010-11-12 01:26:48eric.araujosettitle: let's equip ftplib.FTP with __enter__ and __exit__ -> context managerment support in imaplib, smtplib, ftplib
stage: needs patch
2010-05-10 15:00:56giampaolo.rodolasetmessages: + msg105440
2010-05-09 19:57:20eric.araujosetmessages: + msg105415
2010-05-09 19:50:22giampaolo.rodolasetfiles: - ftplib.patch
2010-05-09 19:50:13giampaolo.rodolasetfiles: + ftplib.patch

messages: + msg105414
2010-05-09 19:35:33eric.araujosetmessages: + msg105411
2010-05-09 19:25:43giampaolo.rodolasetmessages: + msg105410
2010-05-09 19:10:24eric.araujosetnosy: + eric.araujo
messages: + msg105409
2010-05-09 19:02:40giampaolo.rodolasetfiles: + ftplib.patch

messages: + msg105408
versions: + Python 3.2, - Python 3.1, Python 2.7
2010-05-09 17:14:40stuaxosetmessages: + msg105403
2010-05-08 14:31:19giampaolo.rodolasetmessages: + msg105293
2010-05-08 14:30:58giampaolo.rodolasetmessages: - msg105292
2010-05-08 14:30:45giampaolo.rodolasetmessages: + msg105292
2009-10-13 02:04:17ezio.melottisetpriority: normal
nosy: + ezio.melotti
messages: + msg93915

2009-10-13 01:57:02stuaxosetnosy: + stuaxo
messages: + msg93914
2009-01-23 01:44:22giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg80393
2009-01-21 22:11:29tareksetmessages: + msg80352
2009-01-21 21:57:59pitrousetnosy: + pitrou
messages: + msg80351
2009-01-21 21:52:29tareksetmessages: + msg80348
2009-01-21 11:21:15tareksetfiles: - smtplib.patch
2009-01-21 11:21:03tareksetfiles: + smtplib.diff
messages: + msg80321
2009-01-21 10:44:07tareksetfiles: - smtplib.patch
2009-01-21 02:27:52belopolskysetmessages: + msg80314
2009-01-20 21:08:12tareksetfiles: + imaplib.diff
2009-01-20 21:07:53tareksetfiles: + smtplib.patch
messages: + msg80280
2009-01-19 21:21:57belopolskysetmessages: + msg80210
2009-01-19 19:03:48tareksetfiles: + ftplib.diff
2009-01-19 19:01:23tareksetmessages: + msg80203
2009-01-19 18:46:12belopolskysetnosy: + belopolsky
messages: + msg80202
2009-01-19 10:43:27tareksetfiles: + smtplib.patch
keywords: + patch
messages: + msg80158
2009-01-17 17:28:09tarekcreate