Skip to content
This repository was archived by the owner on Jul 26, 2026. It is now read-only.

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

Merged
Byron merged 2 commits into from
Jan 2, 2025
+18 −4
Merged

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

Changes from all commits
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
22 changes: 18 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:
Comment thread
DEKHTIARJonathan marked this conversation as resolved.
os.remove(tmp_path)
remove(tmp_path)
raise
# END assure tmpfile removal on error

Expand All @@ -228,9 +229,22 @@ 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

@EliahKagan EliahKagan Jan 4, 2025

Copy link
Copy Markdown
Member

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.

@Byron Byron Jan 5, 2025

Copy link
Copy Markdown
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.

@EliahKagan EliahKagan Jan 26, 2025

Copy link
Copy Markdown
Member

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

# END handle dry_run

istream.binsha = hex_to_bin(hexsha)
Expand Down