Sporadic importlib failures: FileNotFoundError on os.rename()

[vstinner]

BPO	13303
Nosy	@brettcannon, @vsajip, @pitrou, @vstinner
Files	import_atomic_race.diff import_atomic_race-1.diff import_atomic_race-2.diff importlib_write_atomic.patch import_perm.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2011-11-10.21:42:28.997>
created_at = <Date 2011-10-31.11:01:10.486>
labels = ['library']
title = 'Sporadic importlib failures: FileNotFoundError on os.rename()'
updated_at = <Date 2011-11-10.21:42:28.996>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2011-11-10.21:42:28.996>
actor = 'neologix'
assignee = 'none'
closed = True
closed_date = <Date 2011-11-10.21:42:28.997>
closer = 'neologix'
components = ['Library (Lib)']
creation = <Date 2011-10-31.11:01:10.486>
creator = 'vstinner'
dependencies = []
files = ['23568', '23571', '23572', '23597', '23606']
hgrepos = []
issue_num = 13303
keywords = ['patch']
message_count = 19.0
messages = ['146691', '146692', '146693', '146696', '146700', '146711', '146712', '146716', '146720', '146723', '146739', '146843', '146859', '146860', '146877', '146881', '146962', '147315', '147407']
nosy_count = 7.0
nosy_names = ['brett.cannon', 'vinay.sajip', 'pitrou', 'vstinner', 'Arfrever', 'neologix', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue13303'
versions = ['Python 3.3']

[vstinner]

Traceback (most recent call last):
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/regrtest.py", line 1186, in runtest_inner
    indirect_test()
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_ttk_textonly.py", line 11, in test_main
    *runtktests.get_tests(gui=False, packages=['test_ttk']))
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/tkinter/test/runtktests.py", line 65, in get_tests
    for module in get_tests_modules(gui=gui, packages=packages):
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/tkinter/test/runtktests.py", line 50, in get_tests_modules
    "tkinter.test")
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/__init__.py", line 123, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 834, in _gcd_import
    loader.load_module(name)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 460, in load_module
    return self._load_module(fullname)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 164, in decorated
    return fxn(self, module, *args, **kwargs)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 365, in _load_module
    exec(code_object, module.__dict__)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/tkinter/test/test_ttk/test_style.py", line 6, in <module>
    import tkinter.test.support as support
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 460, in load_module
    return self._load_module(fullname)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 164, in decorated
    return fxn(self, module, *args, **kwargs)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 353, in _load_module
    code_object = self.get_code(name)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 447, in get_code
    self.set_data(bytecode_path, data)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 514, in set_data
    _write_atomic(path, data)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/importlib/_bootstrap.py", line 94, in _write_atomic
    _os.rename(path_tmp, path)
FileNotFoundError: [Errno 2] No such file or directory

[neologix]

There's a race in _write_atomic():
"""
# On POSIX-like platforms, renaming is atomic
path_tmp = path + '.tmp'
try:
fd = _os.open(path_tmp, _os.O_EXCL | _os.O_CREAT | _os.O_WRONLY)
with _io.FileIO(fd, 'wb') as file:
file.write(data)
_os.rename(path_tmp, path)
except OSError:
try:
_os.unlink(path_tmp)
except OSError:
pass
raise
"""

Let's pretend a process managed to open the file (with O_EXCL): before it finishes and calls rename(), another process tries to open the file: since it can't create it (with O_EXCL), it jumps to:

        except OSError:
            try:
                _os.unlink(path_tmp)
            except OSError:
                pass
            raise

and unlinks the file.
The first process then calls rename()...

[neologix]

Patch attached.

[pitrou]

This looks good to me. I'm slightly worried about what happens when there's a stale tmp file (for example if the Python process crashed before renaming it).

[neologix]

Then we won't write the bytecode any more.
But it will be consistent.
The solution would be to create a random temporary file (with e.g. mkstemp()): in case of crash, we'll let dangling temporary files, but we'll be able to update the bytecode.
I'm note sure it's worth it (and it won't be easy since we're really restricted in what we're allowed to call).

[neologix]

Here's a patch using pseudo-random filenames.
I used id(path) to generate a random name: it's faster than getpid(), and doesn't pollute strace's output (I'm one of strace's biggest fan :-).

[pitrou]

Here's a patch using pseudo-random filenames.
I used id(path) to generate a random name: it's faster than getpid(),
and doesn't pollute strace's output (I'm one of strace's biggest
fan :-).

If you go that way, you should also modify Python/import.c to use a
similar logic.
(it does sound a bit overkill to me :-))

[neologix]

(it does sound a bit overkill to me :-))

Well, it depends on what we want:

• if having a stale bytecode file indefinitely is acceptable, then the '.tmp' suffix approach is OK
• if not, then we should use pseudo-random files

If you go that way, you should also modify Python/import.c to use a
similar logic.

I think I would use mkstemp(), but yes, that's the idea.

So, what do you think?

[neologix]

Here's a patch for Python/import.c using mkstemp(3). AFAICT, mkstemp should always be available (on Unix of course).

[pitrou]

Here's a patch for Python/import.c using mkstemp(3). AFAICT, mkstemp
should always be available (on Unix of course).

This looks fine to me. The code simplification tastes good :)

[python-dev]

New changeset 740baff4f169 by Charles-François Natali in branch 'default':
Issue bpo-13303: Fix a race condition in the bytecode file creation.
http://hg.python.org/cpython/rev/740baff4f169

[neologix]

mkstemp() creates the file with mode 0600, which can be surprising.
I'm note sure about the best way to handle this:

1. change the file mode after creating it with fchmod(), using the source file mode. But we must also take into account the umask, so we'd have to do:
   mask = umask(0);
   umask(mask);
   [...]
   fd = mkstemp(cpathname_tmp)
   fchmod(fd, mode & ~mask);

The double call to umask() is necessary because we can't just retrieve the umask
2) don't use mkstemp(), and use something like:

sprintf(cpathname_tmp, "%s.%x", cpathname, 0xffff & getpid());

    d = open(cpathname_tmp, O_WRONLY|O_CREAT|O_EXCL);

to mimic what's done in Lib/importlib/_bootstrap.py (pseudo-temp file).

3. Fall back to the original ".tmp" suffix (with the risk of stale tmp file).

Thoughts?