pytest-dev / py

Python development support library (note: maintenance only)
MIT License
69 stars 105 forks source link

make_numbered_dir() multi-process-safe #143

Closed aurzenligl closed 7 years ago

aurzenligl commented 7 years ago

Solves: https://github.com/pytest-dev/py/issues/30

Patch uses ".lock" file as means to gain exclusive access to directory to use or to remove. Both user and remover race to create exclusively ".lock" files in directories starting with "prefix". If user wins, directory shall not be removed during usage. If remover wins, directory shall be "removed atomically", i.e. renamed, so that following race condition doesn't happen: 1) two removers pass condition for removing directory, 2) first is preempted, 3) second removes it, 4) user creates new directory with the same name, 5) first remover removes user's directory.

Patch introduces new dependencies to code: uuid, and to tests: multiprocessing. Both can be found in stdlib as early as 2.6.

Tests which passed at time of forking pass still, one which failed fails still: testing/path/test_svnauth.py::TestSvnWCAuthFunctional::()::test_switch

RonnyPfannschmidt commented 7 years ago

@aurzenligl thanks for this great work, it took a while to find the space of mind to go trough and understand 👍

aurzenligl commented 7 years ago

It's a pleasure to contribute to project one thinks highly of.

RonnyPfannschmidt commented 7 years ago

@nicoddemus i think this is the pr we will have to undo if we cnat sort out the windows issue with the x flag to open

@aurzenligl please take a look at #157 since the improved lockfile handling turns out to trigger issues on windows that i missed

aurzenligl commented 7 years ago

Current code, in order to use .lock-based mutually exclusive access to directory, uses:

lockfile = path.join('.lock')
if hasattr(lockfile, 'mksymlinkto'):
    lockfile.mksymlinkto(str(mypid))
else:
    lockfile.write(str(mypid), 'wx')

Above condition is never true for windows, hence - ironically - the only platform which uses 'wx'-mode open is the one which doesn't support it:

iswin32 = sys.platform == "win32" or (getattr(os, '_name', False) == 'nt')
# ...
FSBase = not iswin32 and PosixPath or common.PathBase

Fortunately, we have low-level file API accessible in both pythons and both win and linux: https://docs.python.org/2/library/os.html#os.open https://docs.python.org/3/library/os.html#os.open

This, instead of lockfile.write should do the trick. It should rethrow py.error.EEXIST to conform to LocalPath APIs though.

try:
    fd = os.open(filename, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0644)
except IOError:
    # ...
with os.fdopen(fd, 'w') as lockfile:
    # ...

@nicoddemus test which checks multi-process safety of make_numbered_dir is: path/test_local.py::test_make_numbered_dir_multiprocess_safe There is no guarantee that it will catch race error, as it depends on filesystem, number of cores, number of loops. It can be said that without 'x' flag code would be neither more nor less safe for multi-process pytest then before pr.

nicoddemus commented 7 years ago

Thanks a lot @aurzenligl, I will give it a spin tomorrow on my other PR!

nicoddemus commented 7 years ago

Thanks @aurzenligl the solution seemed to work (sorry for the delay). Just pushed my changes to #157.