Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ensure_path_nonexistent race condition aware #429

Merged
merged 7 commits into from
Jan 29, 2024
Merged

Make ensure_path_nonexistent race condition aware #429

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1d877b4
Make ensure_path_nonexistent race condition aware
elikoga Dec 20, 2023
7a228e0
add import to random
elikoga Dec 20, 2023
2eb3d0f
ensure_path_nonexistent: use tempfile
elikoga Jan 4, 2023
c243cd6
adjust test for race condition
elikoga Jan 8, 2024
8deb110
fix tests by checking for broken symlinks and readd newlines remove b…
elikoga Jan 9, 2024
405f97b
Add docs, fix ensure_path_nonexistent
elikoga Jan 24, 2024
50b631a
removes a test for ensure_path_nonexistent (race)
elikoga Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 13 additions & 20 deletions src/batou/lib/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import json
import os.path
import pwd
import random
import re
import shutil
import stat
Expand All @@ -19,29 +18,23 @@
from batou.utils import dict_merge


def ensure_path_nonexistent(path):
def ensure_path_nonexistent(path: str) -> None:
"""Ensure that the given path does not exist.
elikoga marked this conversation as resolved.
Show resolved Hide resolved

strategy: rename the file to a temporary name, then remove it.
this allows us to fail in a useful way, since rename is atomic.
If we detect that the file exists after the rename, we know that
something else is going on and we can bail out.
strategy: create a temporary directory, move the path into it
and delete the temporary directory.
"""
tmp_nonce = random.getrandbits(32)
tmp_filename = path + ".batou-ensure-path-nonexistent-tmp" + str(tmp_nonce)
if not os.path.lexists(path):
if not os.path.exists(path):
return
os.rename(path, tmp_filename)
# if the rename did not work, we bail out
assert not os.path.lexists(
path
), f"{path} should not exist, if it does, it is a race condition."
if os.path.islink(tmp_filename):
os.unlink(tmp_filename)
elif os.path.isdir(tmp_filename):
shutil.rmtree(tmp_filename)
else:
os.unlink(tmp_filename)

parent_dir = os.path.dirname(os.path.abspath(path))
path_basename = os.path.basename(os.path.normpath(path))

with tempfile.TemporaryDirectory(dir=parent_dir) as temp_dir:
os.rename(path, os.path.join(temp_dir, path_basename))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fail because the whole idea of a race condition means someone else might be doing unexpected stuff, so failing renames should be ignored if the path to delete is gone in any case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This context manager is for deleting the Temporary Directory. No assumption that os.rename may fail, but the happy path thankfully includes a deletion of the tmpdir

I can split it up if this is misleading

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rename needs to be wrapped in a try/except. Ignore for FileNotFound, re-raise in other cases.

# deletes temp_dir on context manager exit

assert not os.path.exists(path), f"{path} still exists after rename, race?"
elikoga marked this conversation as resolved.
Show resolved Hide resolved


class File(Component):
Expand Down
Loading