[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis :


--
nosy: +Arfrever

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

Good point.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Charles-François Natali]

Charles-François Natali added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

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.

--
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

Proposed patch for 2.7

--
Added file: http://bugs.python.org/file30324/13146-2.7.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Changes by Barry A. Warsaw :


--
nosy: +doko

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Christian Heimes]

Changes by Christian Heimes :


--
nosy: +christian.heimes

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Barry A. Warsaw]

Changes by Barry A. Warsaw :


--
nosy: +barry

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Brett Cannon]

Brett Cannon  added the comment:

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  wrote:
>
> Antoine Pitrou  added the comment:
>
> Should be fixed now. Thanks for the reviews!
>
> --
> resolution:  -> fixed
> stage: patch review -> committed/rejected
> status: open -> closed
>
> ___
> Python tracker 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Should be fixed now. Thanks for the reviews!

--
resolution:  -> fixed
stage: patch review -> committed/rejected
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Roundup Robot]

Roundup Robot  added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou  added the comment:

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.

--
versions:  -Python 3.2
Added file: http://bugs.python.org/file23396/importrename3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[STINNER Victor]

STINNER Victor  added the comment:

> with atomic_create(, 'b') as f:

See issues #8604 and #8828.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Charles-François Natali]

Charles-François Natali  added the comment:

> 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 .
- 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(, 'b') as f:
f.write()

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Charles-François Natali]

Charles-François Natali  added the comment:

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

--
nosy: +neologix

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[STINNER Victor]

STINNER Victor  added the comment:

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?

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou  added the comment:

This new patch also fixes importlib.

--
stage:  -> patch review
versions:  -Python 2.7
Added file: http://bugs.python.org/file23378/importrename2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Here is a patch for import.c.

--
keywords: +patch
Added file: http://bugs.python.org/file23375/importrename.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue13146] Writing a pyc file is not atomic

[Antoine Pitrou]

New submission from Antoine Pitrou :

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 
import http.client
  File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/http/client.py", line 
69, in 
import email.parser
  File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/email/parser.py", line 
12, in 
from email.feedparser import FeedParser
  File "/var/lib/buildslave/3.x.murray-gentoo/build/Lib/email/feedparser.py", 
line 28, in 
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.

--
components: Interpreter Core
messages: 145313
nosy: brett.cannon, ncoghlan, pitrou, r.david.murray
priority: normal
severity: normal
status: open
title: Writing a pyc file is not atomic
type: behavior
versions: Python 2.7, Python 3.2, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com