Possible race-condition while writing bytecode when rewriting assertions

[nicoddemus]

Since we started porting to Python 3, our builds on Windows fail sometimes, say 5-10% of the runs, in a bizarre and random manner:

___________ ERROR at setup of testMeshAssemblyPlaneControllerNoBBox ___________
[gw3] win32 -- Python 3.5.3 W:\Miniconda\envs\_sci20-win64g-py35\python.exe
file K:\etk\sci20\source\python\sci20\app\mesh_assembly\plane\_tests\test_mesh_assembly_plane_controller.py, line 83
  def testMeshAssemblyPlaneControllerNoBBox(almost_equal):
E       fixture 'py5' not found

Strangely the "name" being reported is always py and some digits, like py2, py7 or py44.

This always happens when running with xdist and in a random test or contest file, which suggest that the assertion rewriting code might some bug in its locking code when writing the rewritten bytecode to the disk, corrupting it.

[nicoddemus]

Also, it seems it always changes an identifier to something else, rather than corrupting actual code.

[nicoddemus]

The code responsible for writing the pyc files is this:

pytest/_pytest/assertion/rewrite.py

Lines 341 to 352 in e012dbe

	def _make_rewritten_pyc(state, source_stat, pyc, co):
	"""Try to dump rewritten code to *pyc*."""
	if sys.platform.startswith("win"):
	# Windows grants exclusive access to open files and doesn't have atomic
	# rename, so just write into the final file.
	_write_pyc(state, co, source_stat, pyc)
	else:
	# When not on windows, assume rename is atomic. Dump the code object
	# into a file specific to this process and atomically replace it.
	proc_pyc = pyc + "." + str(os.getpid())
	if _write_pyc(state, co, source_stat, proc_pyc):
	os.rename(proc_pyc, pyc)

Unfortunately the comment about Windows is not correct, there's no exclusive access guarantee and it is possible for concurrent processes/threads to write to a file at the same time and possibly corrupt it.

This short script demonstrates this:

import os
import tempfile
import threading

with tempfile.TemporaryDirectory() as tmpdir:
    filename = os.path.join(tmpdir, 'somefile.txt')
    print(f'FILE: {filename}')

    COUNT = 10

    data = [1025 * 1024 * str(i) for i in range(COUNT)]


    def write(i):
        with open(filename, 'w') as f:
            f.write(data[i])

    threads = [threading.Thread(target=write, args=(i,)) for i in range(COUNT)]
    for t in threads:
        t.start()

    for t in threads:
        t.join()

    print('DONE')
    
    with open(filename) as f:
        read_data = set(f.read())

    print(f'READ: {read_data}')

It spawns a number of threads, all of them writing different data to the same file. At the end, it counts how many different characters were written to the file.

Most of the time the output is this:

FILE: C:\Users\Bruno\AppData\Local\Temp\tmpzzgwfa99\somefile.txt
DONE
READ: {'2'}

But sometimes (2/9 on my machine) the file ends up corrupted:

FILE: C:\Users\Bruno\AppData\Local\Temp\tmpm0o7bpuq\somefile.txt
DONE
READ: {'8', '9'}

This behavior is consistent with the CI failures we have seen: corrupted .pyc files around 2-3% of the time.

I searched for some solutions about the problem of atomically renaming a file on multiple platforms and the best library I found was python-atomicwrites, which is small and does just this job.

[nicoddemus]

Hopefully fixed in #3390. Will re-open if I see this problem again in pytest>=3.6.

[maiksensi]

great job for debugging this. Can't actually 👍 enough! Thanks!

[nicoddemus]

@maiksensi were you experiencing this as well?