Mailing List Archive

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

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)

----------
keywords: +patch
Added file: http://bugs.python.org/file12792/smtplib.patch

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

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).

----------
nosy: +belopolsky

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

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()


> 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).

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Changes by Tarek Ziadé <ziade.tarek@gmail.com>:


Added file: http://bugs.python.org/file12799/ftplib.diff

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

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.

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

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

> 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.

Added file: http://bugs.python.org/file12811/smtplib.patch

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Changes by Tarek Ziadé <ziade.tarek@gmail.com>:


Added file: http://bugs.python.org/file12812/imaplib.diff

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

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.

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Changes by Tarek Ziadé <ziade.tarek@gmail.com>:


Removed file: http://bugs.python.org/file12792/smtplib.patch

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

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

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

Added file: http://bugs.python.org/file12823/smtplib.diff

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Changes by Tarek Ziadé <ziade.tarek@gmail.com>:


Removed file: http://bugs.python.org/file12811/smtplib.patch

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

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

Feedback from my blog :

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

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

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Antoine Pitrou <pitrou@free.fr> added the comment:

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

----------
nosy: +pitrou

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

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

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)

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Giampaolo Rodola' <billiejoex@users.sourceforge.net> added the comment:

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()

----------
nosy: +giampaolo.rodola

_______________________________________
Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue4972>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com