msg80026 - (view) |
Author: Tarek Ziadé (tarek) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2009-10-13 02:04 |
There's a patch for zipfile in #5511.
|
msg105293 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
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) * |
Date: 2010-05-09 19:02 |
A patch including tests and doc changes is in attachment.
|
msg105409 - (view) |
Author: Éric Araujo (eric.araujo) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
msg223228 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2014-07-16 16:01 |
Updated Serhiy's patch to 3.5. I also added a whatsnew entry.
|
msg226640 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-09-09 16:00 |
Thanks Berker.
|
msg226642 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-09-09 16:14 |
I think that's all with this issue.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:44 | admin | set | github: 49222 |
2014-09-09 16:14:43 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages:
+ msg226642
stage: patch review -> resolved |
2014-09-09 16:00:03 | serhiy.storchaka | set | assignee: serhiy.storchaka messages:
+ msg226640 |
2014-07-16 16:08:05 | r.david.murray | set | nosy:
+ r.david.murray
|
2014-07-16 16:01:35 | berker.peksag | set | files:
+ issue4972_imaplib.diff
messages:
+ msg223228 |
2014-07-04 15:34:48 | berker.peksag | set | versions:
+ Python 3.5, - Python 3.4 |
2013-02-16 22:14:00 | serhiy.storchaka | set | files:
+ imaplib_with_3.patch
messages:
+ msg182244 |
2013-01-27 19:42:42 | berker.peksag | set | nosy:
+ berker.peksag
messages:
+ msg180785 title: context managerment support in imaplib, smtplib, ftplib -> context management support in imaplib, smtplib, ftplib |
2013-01-27 10:52:50 | serhiy.storchaka | set | files:
+ imaplib_with_2.patch nosy:
+ serhiy.storchaka messages:
+ msg180761
|
2013-01-27 01:31:11 | ezio.melotti | set | stage: needs patch -> patch review messages:
+ msg180730 versions:
+ Python 3.4, - Python 3.2, Python 3.3 |
2011-03-15 18:33:13 | SilentGhost | set | versions:
+ Python 3.3 nosy:
+ SilentGhost
messages:
+ msg131015
dependencies:
+ smtplib context manager |
2010-11-12 01:26:48 | eric.araujo | set | title: let's equip ftplib.FTP with __enter__ and __exit__ -> context managerment support in imaplib, smtplib, ftplib stage: needs patch |
2010-05-10 15:00:56 | giampaolo.rodola | set | messages:
+ msg105440 |
2010-05-09 19:57:20 | eric.araujo | set | messages:
+ msg105415 |
2010-05-09 19:50:22 | giampaolo.rodola | set | files:
- ftplib.patch |
2010-05-09 19:50:13 | giampaolo.rodola | set | files:
+ ftplib.patch
messages:
+ msg105414 |
2010-05-09 19:35:33 | eric.araujo | set | messages:
+ msg105411 |
2010-05-09 19:25:43 | giampaolo.rodola | set | messages:
+ msg105410 |
2010-05-09 19:10:24 | eric.araujo | set | nosy:
+ eric.araujo messages:
+ msg105409
|
2010-05-09 19:02:40 | giampaolo.rodola | set | files:
+ ftplib.patch
messages:
+ msg105408 versions:
+ Python 3.2, - Python 3.1, Python 2.7 |
2010-05-09 17:14:40 | stuaxo | set | messages:
+ msg105403 |
2010-05-08 14:31:19 | giampaolo.rodola | set | messages:
+ msg105293 |
2010-05-08 14:30:58 | giampaolo.rodola | set | messages:
- msg105292 |
2010-05-08 14:30:45 | giampaolo.rodola | set | messages:
+ msg105292 |
2009-10-13 02:04:17 | ezio.melotti | set | priority: normal nosy:
+ ezio.melotti messages:
+ msg93915
|
2009-10-13 01:57:02 | stuaxo | set | nosy:
+ stuaxo messages:
+ msg93914
|
2009-01-23 01:44:22 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola messages:
+ msg80393 |
2009-01-21 22:11:29 | tarek | set | messages:
+ msg80352 |
2009-01-21 21:57:59 | pitrou | set | nosy:
+ pitrou messages:
+ msg80351 |
2009-01-21 21:52:29 | tarek | set | messages:
+ msg80348 |
2009-01-21 11:21:15 | tarek | set | files:
- smtplib.patch |
2009-01-21 11:21:03 | tarek | set | files:
+ smtplib.diff messages:
+ msg80321 |
2009-01-21 10:44:07 | tarek | set | files:
- smtplib.patch |
2009-01-21 02:27:52 | belopolsky | set | messages:
+ msg80314 |
2009-01-20 21:08:12 | tarek | set | files:
+ imaplib.diff |
2009-01-20 21:07:53 | tarek | set | files:
+ smtplib.patch messages:
+ msg80280 |
2009-01-19 21:21:57 | belopolsky | set | messages:
+ msg80210 |
2009-01-19 19:03:48 | tarek | set | files:
+ ftplib.diff |
2009-01-19 19:01:23 | tarek | set | messages:
+ msg80203 |
2009-01-19 18:46:12 | belopolsky | set | nosy:
+ belopolsky messages:
+ msg80202 |
2009-01-19 10:43:27 | tarek | set | files:
+ smtplib.patch keywords:
+ patch messages:
+ msg80158 |
2009-01-17 17:28:09 | tarek | create | |