homepage

One of the buildbots recently showed the following failure:
======================================================================
ERROR: test_rapid_restart (test.test_multiprocessing.WithProcessesTestManagerRestart)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/test/test_multiprocessing.py", line 1442, in test_rapid_restart
 queue = manager.get_queue()
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/multiprocessing/managers.py", line 670, in temp
 token, exp = self._create(typeid, *args, **kwds)
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/multiprocessing/managers.py", line 568, in _create
 conn = self._Client(self._address, authkey=self._authkey)
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/multiprocessing/connection.py", line 778, in XmlClient
 import xmlrpc.client as xmlrpclib
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/xmlrpc/client.py", line 137, in <module>
 import http.client
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/http/client.py", line 69, in <module>
 import email.parser
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/email/parser.py", line 12, in <module>
 from email.feedparser import FeedParser
 File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/email/feedparser.py", line 28, in <module>
 from email import policy
EOFError: EOF read where not expected
(http://www.python.org/dev/buildbot/all/builders/x86%20Gentoo%203.x/builds/942/steps/test/logs/stdio)
"EOF read where not expected" comes from reading a pyc file in marshal.c. It is raised when the pyc file is somehow truncated or incomplete. Writing and reading the same pyc file is protected by the import lock when in a single interpreter, but not when running several Python processes at the same time (which test_multiprocessing obviously does).
Under POSIX, import.c could do the traditional write-then-rename dance which guarantees that the file contents appear atomically.
And so could importlib.

Here is a patch for import.c.

This new patch also fixes importlib.

So if a process replaces the PYC file whereas another is reading the PYC, the reader may read corrupted data? The ideal fix is maybe to use a file lock?

> So if a process replaces the PYC file whereas another is reading the
> PYC, the reader may read corrupted data?
No, this is the whole point of the patch.

> Here is a patch for import.c.
Looks good to me.
> This new patch also fixes importlib.
"""
 path_tmp = path + '.tmp'
 with _io.FileIO(path_tmp, 'wb') as file:
 file.write(data)
 _os.rename(path_tmp, path)
"""
I don't know exactly the context in which this code runs, but you can have a corruption if multiple processes try to write the bytecode file at the same time, since they'll all open the .tmp file: it should be opened with O_EXCL.
Also, as a side note, I'm wondering whether this type of check:
"""
 if not sys.platform.startswith('win'):
 # On POSIX-like platforms, renaming is atomic
"""
couldn't be rewritten as
"""
 if os.name == 'posix':
 # On POSIX-like platforms, renaming is atomic
"""
Fox example, does OS-X report as POSIX?

> > This new patch also fixes importlib.
> 
> """
> path_tmp = path + '.tmp'
> with _io.FileIO(path_tmp, 'wb') as file:
> file.write(data)
> _os.rename(path_tmp, path)
> """
> 
> I don't know exactly the context in which this code runs, but you can
> have a corruption if multiple processes try to write the bytecode file
> at the same time, since they'll all open the .tmp file: it should be
> opened with O_EXCL.
Or perhaps append the PID to the name of the temp file ?
(easier done in Python than in C :-))
> Also, as a side note, I'm wondering whether this type of check:
> """
> if not sys.platform.startswith('win'):
> # On POSIX-like platforms, renaming is atomic
> """
> 
> couldn't be rewritten as
> """
> if os.name == 'posix':
> # On POSIX-like platforms, renaming is atomic
> """
No, because os.py is not available to importlib (which must be
bootstrappable early). See the _bootstrap.py header for information
about what is available; this is also why we use FileIO instead of
open().
> Fox example, does OS-X report as POSIX?
I think so.

> Or perhaps append the PID to the name of the temp file ?
> (easier done in Python than in C :-))
I don't really like appending PIDs to generate file names:
- if you have multiple processes at the same time, they'll all write
their own file which will end up being replaced by the last one to
perform the move, whereas with O_EXCL, they'll see immediately that
another instance is writing it (the overhead is negligible with such
small files, but maybe not so much when creating the file requires a
certain amout of work)
- if processes crash at the wrong time, you can end up with a flurry
of <filename>.<PID>
- the last one is even more insidious and unlikely, but here it goes:
the PID is unique only on a given machine: if you have, for example, a
network file system shared between multiple hosts, then you can have a
PID collision, whereas O_EXCL is safe (O_EXCL doesn't work on NFSv2,
but nowadays every OS implements it correctly on NFSv3)
O_EXCL is really what POSIX offers to solve this (and it's also what
import.c does).
>
>> Also, as a side note, I'm wondering whether this type of check:
>> """
>> if not sys.platform.startswith('win'):
>> # On POSIX-like platforms, renaming is atomic
>> """
>>
>> couldn't be rewritten as
>> """
>> if os.name == 'posix':
>> # On POSIX-like platforms, renaming is atomic
>> """
>
> No, because os.py is not available to importlib (which must be
> bootstrappable early). See the _bootstrap.py header for information
> about what is available; this is also why we use FileIO instead of
> open().
OK. So is the O_EXCL approach possible? Would something like
_io.open(_os.open(path, _os.O_CREATE|os.O_EXCL...), 'wb')
work?
Also, since this can be quite tricky and redundant, how about adding a
framework to do this kind of thing to the standard library?
Something like
with atomic_create(<final path>, 'b') as f:
 f.write(<data>)
where atomic_create would be a context manager that would make `f`
point to a temporary file (open with O_EXCL :-), and do the rename at
the end. It could also accept an option to ensure durability (i.e.
call fsync() on the file and on the parent directory). Note that it
probably wouldn't help here, since we only have access to a really
limited part of the library :-)

> with atomic_create(<final path>, 'b') as f:
See issues #8604 and #8828.

Ok, here is a new patch using O_EXCL.
Also, since import.c is quite different in 3.2, I'm not sure I will bother backporting.

New changeset c16063765d3a by Antoine Pitrou in branch 'default':
Issue #13146: Writing a pyc file is now atomic under POSIX.
http://hg.python.org/cpython/rev/c16063765d3a 

Should be fixed now. Thanks for the reviews!

And thanks for doing this, Antoine! One less thing on my never-ending
todo list. =)
On Mon, Oct 17, 2011 at 10:35, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Should be fixed now. Thanks for the reviews!
>
> ----------
> resolution: -> fixed
> stage: patch review -> committed/rejected
> status: open -> closed
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue13146>
> _______________________________________
>

Proposed patch for 2.7

I'm re-opening this because I'd like to get RM pronouncement on applying a patch to 2.7, 3.2, and 3.3 to make py_compile.py atomically rename its pyc/pyo file.
Attached is a patch for 2.7 based on importlib's approach in 3.4. It should be easy enough to port to Python 3.

Oh btw, if Georg and Benjamin deny this for the stable releases, I'll very likely patch the Ubuntu versions anyway.

> I'm re-opening this because I'd like to get RM pronouncement on
> applying a patch to 2.7, 3.2, and 3.3 to make py_compile.py atomically
> rename its pyc/pyo file.
Some people already complained about this change. I'm not sure it's fit
for a bugfix release.
http://bugs.python.org/issue17222
Besides, you can just also make py_compile write to a temporary file,
then do the rename yourself.

On May 20, 2013, at 09:52 PM, Antoine Pitrou wrote:
>Some people already complained about this change. I'm not sure it's fit for a
>bugfix release. http://bugs.python.org/issue17222
Yeah, but that's a crazy use case. :)
>Besides, you can just also make py_compile write to a temporary file,
>then do the rename yourself.
That actually doesn't work as well for us, since we feed .py file names to
py_compile via stdin. I'd rather rename them atomically on a per-file basis
rather than at the end of all the byte compilations.

IIRC, os.rename() will fail on Windows if the target file already exists.
That's why os.replace() was added.

> IIRC, os.rename() will fail on Windows if the target file already
> exists.
Good point.

On May 20, 2013, at 09:57 PM, Charles-François Natali wrote:
>IIRC, os.rename() will fail on Windows if the target file already exists.
>That's why os.replace() was added.
Ah, that's probably a more serious blocker for adding it to upstream Python.
Not so for fixing it in Debian/Ubuntu though!

The workaround would be to unlink the file first, and then try to
create it with O_EXCL. You have a short window where there's no file,
but that shouldn't be a problem in this specific case, and it would
work on Windows.
As for issue #17222, well, many applications use temporary files and
rename (e.g. most web browsers), so I'd be tempted to say "don't do
it".
Of course, I would feel kinda bad if Python broke Debian's builders (I
don't care about Gentoo though ;-)
Its funny how an seemingly harmless change can introduce nasty regressions...

Barry, do you still want to keep this issue open?

On Aug 15, 2013, at 08:25 PM, Antoine Pitrou wrote:
>Barry, do you still want to keep this issue open?
I don't necessarily need to. We've patched the Ubuntu version to be safe, so
I guess we'll just carry that delta along until 3.4.