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

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

Patch attached.

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

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

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

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

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

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

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

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

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?

What's the downside of option 2) ?

> Thoughts?

I would say go for the simpler (that's probably option 3).

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

I missed something, what is the "stale tmp file" issue? Python/import.c uses:

    (void) unlink(filename);
    fd = open(filename, O_EXCL|O_CREAT|O_WRONLY|O_TRUNC ...);

So Python starts by removing the .tmp file, but it fails if another process is already writing into the .tmp file. In this case, we do nothing, which is not a problem: the other process will create the file.

Attached patch implements the same algorithm than import.c in importlib.

> So Python starts by removing the .tmp file, but it fails if another 
> process is already writing into the .tmp file. In this case, we do 
> nothing, which is not a problem: the other process will create the 
> file.

unlink() does not fail, even if the file is open by another process with O_EXCL!
Therefore there's a race:
- process 1 opens file.tmp
- process 2 unlinks file.tmp
- process 2 opens file.tmp: this succeeds, since he just removed the file opened by proc 1
- process 1, which was working on its deleted file handle, is done, and renames file.tmp to file: except that it rename the file process 2 is in the middle of writing
- game over, file corrupted

> Attached patch implements the same algorithm than import.c in 
> importlib. 

Same race.


The current implementations are safe, both Python/import.c and Lib/importlib/_bootstrap.py
The only problem is that since import.c uses mkstemp, the file is created with mode 0600.

Here's a patch using the '.tmp' suffix.
I also updated test_import.
Note that the current test_import.test_execute_bit_not_copied is a no-op:
"""
                fname = TESTFN + os.extsep + "py"
                create_empty_file(fname)
                os.chmod(fname, (stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH |
                                 stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH))
                __import__(TESTFN)
                fn = imp.cache_from_source(fname)
                if not os.path.exists(fn):
                    self.fail("__import__ did not result in creation of "
                              "either a .pyc or .pyo file")
                    s = os.stat(fn)
                    self.assertEqual(
                        stat.S_IMODE(s.st_mode),
                        stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH)

"""

The indentation is wrong: the stat() is never performed...
I'll fix that in 3.2 and 2.7.

New changeset 1238cdd288d2 by Charles-François Natali in branch 'default':
Back out changeset b6336ba796d4 until fix for #13303.
http://hg.python.org/cpython/rev/1238cdd288d2

New changeset a9f10c3eff69 by Charles-François Natali in branch 'default':
Issue #13303: Fix bytecode file default permission.
http://hg.python.org/cpython/rev/a9f10c3eff69