classification
Title: 2.6 stdlib using with statement
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 2.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Rhamphoryncus, benjamin.peterson, christian.heimes, lemburg, rhettinger, theller
Priority: normal Keywords: patch

Created on 2008-01-26 19:21 by benjamin.peterson, last changed 2009-02-17 15:17 by exarkun. This issue is now closed.

Files
File name Uploaded Description Edit
stdlib-with-stmt.diff benjamin.peterson, 2008-01-26 19:21
stdlib-with-stmt2.diff benjamin.peterson, 2008-01-26 20:39
stdlib-with-stmt2-no-platform.diff benjamin.peterson, 2008-02-01 02:23
Messages (20)
msg61713 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-01-26 19:21
This patch modernizes many modules in the stdlib by making them using
the with statement. They affected modules are modulefinder, ftplib,
cookielib, shutil, pydoc, platform, _LWPCookieJar, mailbox,
_MozillaCookieJar, and zipfile.
msg61714 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-01-26 20:05
modulefinder should be kept compatible with Python 2.2, so please do not
apply the patch for this module.  See also PEP 291.

No idea about the other modules.
msg61715 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-26 20:15
A while ago Raymond explained on Python developer list that the with
statement is slightly slower than a try/finally block. Performance
critical sections like the threading module must not use the with statement.
msg61716 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2008-01-26 20:26
The patch doesn't change the threading module, so I'm not sure if
there's anything in particular you think is performance critical there.
 The places where it uses try/finally are:

  * _Condition.wait.  This performs operations on a mutex which are much
slower than any overhead "with" might bring.  It also has sleep calls in
it to implement timeout support, so it's not going to be extremely fast
in those cases anyway.  I notice, however, that it executes a number of
lines of code outside before setting up the try/finally statement,
allowing for the possibility of a buggy deadlock should an exception
(such as KeyboardInterrupt) occur before the try block begins.
  * _Event.set, which uses _Condition and so also deals with mutexes.
  * _Event.clear and _Event.wait, similarly.
  * Thread.__bootstrap, which writes to stderr and uses mutexes, so is
already quite slow. (but this case probably isn't a candidate for
conversion to with anyway)
  * Thread.__delete and Thread.join,  - more mutexes and conditions.

So there doesn't seem to be anything particularly performance critical
here.  You can argue that the slightly increased overhead of with would
still be bad, even for things that are already slow (such as mutex
manipulation), I suppose.  In this case, measuring the difference would
be a useful step to take.

However, there's also the issue of correctness.  A KeyboardInterrupt can
arrive between any of these mutxes being acquired and the try/finally
being set up.  It can also arrive after the finally begins executing and
before the mutex is actually released.  In either case, this will result
in a lock being held when it shouldn't be, which will probably lead to a
deadlock.  Converting to with to manipulate these mutexes (assuming the
lock type has its context management methods implemented in C) will
remove the possibility of these bugs occurring.
msg61717 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-01-26 20:36
Is there a guarantee that the with-statement is safe in the face of
KeyboardInterrupt?  PEP 343 seems to imply it's not, using it as a
reason for why we need no special handling if __exit__ fails.
msg61718 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2008-01-26 20:38
It's explicitly not (and Guido said this is what he wanted, several
times).  However, CPython will not raise a KeyboardInterrupt in the
middle of a C function.  Hence my parenthetical about implementing the
lock type in C.
msg61719 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-01-26 20:39
This new patch removes the modulefinder changes.

I was the one you submitted #1910 which probably sparked the performance
debate. In this patch, I tried to avoid these touchy places.
msg61721 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-01-26 20:49
Yes, but there's no guarantee it will even reach the C function.
msg61722 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2008-01-26 20:51
There may not be a guarantee, but it will with the current
implementation, and the discussion on this ticket seems to be very
geared towards CPython implementation peculiarities.
msg61723 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-01-26 21:50
Please make darned sure these changes are semantically neutral.
msg61725 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-01-26 22:05
Sorry, but what does that mean? All tests are passing with the changes.
msg61727 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-26 22:27
Benjamin Peterson wrote:
> Sorry, but what does that mean? All tests are passing with the changes.

Tests aren't a proof for correctness. A failing test proofs either an
error in the implementation or in the test suite. The reverse "all tests
are passing => everything is fine" is not a valid assumption.

Every change must be carefully studied. with open(name) as fp is fine in
almost every case but other cases may not.
msg61731 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-01-26 23:27
It means to that the test suites are likely inadequate in this 
department and that you need to carefully check each transformation to 
make sure it doesn't do something subtlely different.
msg61745 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-01-27 18:58
Today, I carefully looked through every change in my patching. I asked
myself "Does this do the same things in the same order as the original?"
and "Could exceptions cause the code to function differently?" They only
changes were when the block which used the file was not in a try finally
block so if an exception were raised, it the file would be closed. The
with statement of course removes this problem. So, I do believe the
patch is "semantically neutral." I would, however, not mind if another
set of eyes examined it.
msg61912 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-01-31 17:32
Comments?
msg61915 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-01-31 17:45
Lib/platform.py contains this notice at the top:

#    This module is maintained by Marc-Andre Lemburg <mal@egenix.com>.
#    If you find problems, please submit bug reports/patches via the
#    Python SourceForge Project Page and assign them to "lemburg".
#
#    Note: Please keep this module compatible to Python 1.5.2.

I didn't look into the other modules in the patch.
msg61917 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2008-01-31 17:48
Please remove the platform.py part of the patch. You can apply that to
Py3k, but not to the 2.x series.
msg61944 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-01 02:23
This new patch removes changes to the platform module.
msg61946 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-01 02:58
Am unassigning because I don't have time for a detailed review.

For the most part, I'm not excited about this patch which targets 
modules that aren't being actively supervised by their original 
contributor.
msg62027 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-03 21:28
I could break this patch into per-module patches. Then as authors
approved them, they could gradually be committed.
History
Date User Action Args
2009-02-17 15:17:45exarkunsetnosy: - exarkun
2009-02-17 01:38:31benjamin.petersonsetstatus: open -> closed
resolution: rejected
2008-02-03 21:28:06benjamin.petersonsetmessages: + msg62027
2008-02-01 02:58:12rhettingersetassignee: rhettinger ->
messages: + msg61946
2008-02-01 02:23:52benjamin.petersonsetfiles: + stdlib-with-stmt2-no-platform.diff
messages: + msg61944
2008-01-31 17:48:25lemburgsetnosy: + lemburg
messages: + msg61917
2008-01-31 17:45:11thellersetmessages: + msg61915
2008-01-31 17:32:14benjamin.petersonsetmessages: + msg61912
2008-01-27 18:58:16benjamin.petersonsetmessages: + msg61745
2008-01-26 23:27:38rhettingersetmessages: + msg61731
2008-01-26 22:27:34christian.heimessetmessages: + msg61727
2008-01-26 22:05:20benjamin.petersonsetmessages: + msg61725
2008-01-26 21:50:41rhettingersetmessages: + msg61723
2008-01-26 20:51:41exarkunsetmessages: + msg61722
2008-01-26 20:49:54Rhamphoryncussetmessages: + msg61721
2008-01-26 20:39:47benjamin.petersonsetfiles: + stdlib-with-stmt2.diff
messages: + msg61719
2008-01-26 20:38:35exarkunsetmessages: + msg61718
2008-01-26 20:36:41Rhamphoryncussetnosy: + Rhamphoryncus
messages: + msg61717
2008-01-26 20:26:26exarkunsetnosy: + exarkun
messages: + msg61716
2008-01-26 20:15:13christian.heimessetnosy: + rhettinger, christian.heimes
messages: + msg61715
priority: normal
assignee: rhettinger
keywords: + patch
type: enhancement
2008-01-26 20:05:27thellersetnosy: + theller
messages: + msg61714
2008-01-26 19:21:27benjamin.petersoncreate