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

Potential Race Condition Fix - OS Rename & Chmod - PermissionError #115

Merged
merged 2 commits into from
Jan 2, 2025
Merged

Potential Race Condition Fix - OS Rename & Chmod - PermissionError #115

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74a0eab
Potential Race Condition Fix - OS Rename & Chmod
DEKHTIARJonathan Dec 24, 2024
b71e273
Update gitdb/db/loose.py
DEKHTIARJonathan Dec 30, 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
25 changes: 21 additions & 4 deletions gitdb/db/loose.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import tempfile
import os
import sys
import time


__all__ = ('LooseObjectDB', )
Expand Down Expand Up @@ -205,7 +206,7 @@ def store(self, istream):
# END assure target stream is closed
except:
if tmp_path:
DEKHTIARJonathan marked this conversation as resolved.
Show resolved Hide resolved
os.remove(tmp_path)
remove(tmp_path)
raise
# END assure tmpfile removal on error

Expand All @@ -228,9 +229,25 @@ def store(self, istream):
rename(tmp_path, obj_path)
# end rename only if needed

# make sure its readable for all ! It started out as rw-- tmp file
# but needs to be rwrr
chmod(obj_path, self.new_objects_mode)
# Ensure rename is actually done and file is stable
# Retry up to 14 times - exponential wait & retry in ms.
# The total maximum wait time is 1000ms, which should be vastly enough for the
# OS to return and commit the file to disk.
for exp_backoff_ms in [1, 4, 9, 16, 25, 36, 49, 64, 81, 100, 121, 144, 169, 181]:
Comment on lines +232 to +236
Copy link
Contributor

Choose a reason for hiding this comment

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

These timings seem reasonable, but I believe this is quadratic backoff rather than exponential backoff. (In exponential backoff, delays are a fixed base, often 2, raised to the power of a number that increases by one with each attempt. Here, the number that increases by one with each attempt is the base, raised to a fixed power of 2.) I mention this only in case true exponential backoff is intended or required.

Copy link
Member

Choose a reason for hiding this comment

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

That is very true! Since I copied these values from gitoxide where I implemented that based on what Git does. So most definitely it was me misnaming this, as Git correctly calls it quadratic.

A PR would definitely be welcome that fixes the name in gitoxide.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened GitoxideLabs/gitoxide#1815 to fix that in gitoxide. While I was at it, I also opened #119 to make the much more minor correction here.

with suppress(PermissionError):
# make sure its readable for all ! It started out as rw-- tmp file
# but needs to be rwrr
chmod(obj_path, self.new_objects_mode)
break
time.sleep(exp_backoff_ms / 1000.0)
else:
raise PermissionError(
"Impossible to apply `chmod` to file {}".format(obj_path)
)

# Cleanup
with suppress(FileNotFoundError):
remove(tmp_path)
DEKHTIARJonathan marked this conversation as resolved.
Show resolved Hide resolved
# END handle dry_run

istream.binsha = hex_to_bin(hexsha)
Expand Down
Loading