Issue 13146: Writing a pyc file is not atomic (original) (raw)

Issue13146

process

Created on 2011-10-10 19:37 by pitrou, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (24)
msg145313 - (view)	Author: Antoine Pitrou (pitrou) *	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 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.
msg145326 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2011-10-11 02:12
Here is a patch for import.c.
msg145362 - (view)	Author: Antoine Pitrou (pitrou) *	Date: 2011-10-11 19:55
This new patch also fixes importlib.
msg145368 - (view)	Author: STINNER Victor (vstinner) *	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) *	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) *	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) *	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) *	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 . - 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 :-)
msg145435 - (view)	Author: STINNER Victor (vstinner) *	Date: 2011-10-12 21:49
> with atomic_create(, 'b') as f: See issues #8604 and #8828.
msg145474 - (view)	Author: Antoine Pitrou (pitrou) *	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) *	Date: 2011-10-17 17:35
Should be fixed now. Thanks for the reviews!
msg145978 - (view)	Author: Brett Cannon (brett.cannon) *	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) *	Date: 2013-05-20 21:47
Proposed patch for 2.7
msg189701 - (view)	Author: Barry A. Warsaw (barry) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	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) *	Date: 2013-08-15 20:25
Barry, do you still want to keep this issue open?
msg200169 - (view)	Author: Barry A. Warsaw (barry) *	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.