classification
Title: Writing a pyc file is not atomic
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, barry, brett.cannon, christian.heimes, doko, haypo, ncoghlan, neologix, pitrou, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2011-10-10 19:37 by pitrou, last changed 2015-04-13 19:11 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
importrename.patch pitrou, 2011-10-11 02:12 review
importrename2.patch pitrou, 2011-10-11 19:55 review
importrename3.patch pitrou, 2011-10-13 17:34 review
13146-2.7.patch barry, 2013-05-20 21:47 review
Messages (24)
msg145313 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-10 19:37
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.
msg145326 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-11 02:12
Here is a patch for import.c.
msg145362 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-11 19:55
This new patch also fixes importlib.
msg145368 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-10-11 21:43
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?
msg145371 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-11 21:55
> 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.
msg145413 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-10-12 16:37
> 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?
msg145415 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-12 16:51
> > 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.
msg145430 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-10-12 20:49
> 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 :-)
msg145435 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-10-12 21:49
> with atomic_create(<final path>, 'b') as f:

See issues #8604 and #8828.
msg145474 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-13 17:34
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.
msg145738 - (view) Author: Roundup Robot (python-dev) Date: 2011-10-17 17:34
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
msg145739 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-17 17:35
Should be fixed now. Thanks for the reviews!
msg145978 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-10-19 22:08
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>
> _______________________________________
>
msg189700 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-05-20 21:47
Proposed patch for 2.7
msg189701 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-05-20 21:48
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.
msg189702 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-05-20 21:50
Oh btw, if Georg and Benjamin deny this for the stable releases, I'll very likely patch the Ubuntu versions anyway.
msg189703 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-20 21:52
> 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.
msg189704 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-05-20 21:55
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.
msg189705 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-05-20 21:57
IIRC, os.rename() will fail on Windows if the target file already exists.
That's why os.replace() was added.
msg189706 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-20 21:57
> IIRC, os.rename() will fail on Windows if the target file already
> exists.

Good point.
msg189707 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-05-20 22:00
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!
msg189708 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-05-20 22:14
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...
msg195285 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-15 20:25
Barry, do you still want to keep this issue open?
msg200169 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-10-17 21:07
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.
History
Date User Action Args
2015-04-13 19:11:57pitrousetstatus: open -> closed
2013-10-17 21:07:11barrysetmessages: + msg200169
2013-08-15 20:25:57pitrousetmessages: + msg195285
2013-05-21 08:39:26Arfreversetnosy: + Arfrever
2013-05-20 22:14:26neologixsetmessages: + msg189708
2013-05-20 22:00:02barrysetmessages: + msg189707
2013-05-20 21:57:56pitrousetmessages: + msg189706
2013-05-20 21:57:07neologixsetmessages: + msg189705
2013-05-20 21:55:15barrysetmessages: + msg189704
2013-05-20 21:52:43pitrousetmessages: + msg189703
2013-05-20 21:50:00barrysetmessages: + msg189702
2013-05-20 21:48:30barrysetstatus: closed -> open

messages: + msg189701
2013-05-20 21:47:17barrysetfiles: + 13146-2.7.patch

messages: + msg189700
2013-05-20 18:11:13barrysetnosy: + doko
2013-05-15 23:24:57christian.heimessetnosy: + christian.heimes
2013-05-15 19:30:23barrysetnosy: + barry
2011-12-02 23:33:41brett.cannonlinkissue9663 superseder
2011-10-19 22:08:33brett.cannonsetmessages: + msg145978
2011-10-17 17:35:02pitrousetstatus: open -> closed
resolution: fixed
messages: + msg145739

stage: patch review -> resolved
2011-10-17 17:34:21python-devsetnosy: + python-dev
messages: + msg145738
2011-10-13 17:34:57pitrousetfiles: + importrename3.patch

messages: + msg145474
versions: - Python 3.2
2011-10-12 21:49:28hayposetmessages: + msg145435
2011-10-12 20:49:34neologixsetmessages: + msg145430
2011-10-12 16:51:01pitrousetmessages: + msg145415
2011-10-12 16:37:57neologixsetnosy: + neologix
messages: + msg145413
2011-10-11 21:55:42pitrousetmessages: + msg145371
2011-10-11 21:43:35hayposetnosy: + haypo
messages: + msg145368
2011-10-11 19:55:34pitrousetfiles: + importrename2.patch

stage: patch review
messages: + msg145362
versions: - Python 2.7
2011-10-11 02:12:22pitrousetfiles: + importrename.patch
keywords: + patch
messages: + msg145326
2011-10-10 19:37:54pitroucreate