Skip to content

Commit

Permalink
feat(api): provide more detailed errors on locking
Browse files Browse the repository at this point in the history
Include scope and compoent in the error message so that it gives more
insight where the blocking operation is happening.

Issue #13345
  • Loading branch information
nijel committed Jan 21, 2025
1 parent b3ef4b7 commit fd3c542
Show file tree
Hide file tree
Showing 9 changed files with 86 additions and 21 deletions.
2 changes: 2 additions & 0 deletions weblate/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1596,6 +1596,8 @@ def edit_service_settings_response_serializer(

class ErrorCode423Enum(models.TextChoices):
REPOSITORY_LOCKED = "repository-locked"
COMPONENT_LOCKED = "component-locked"
UNKNOWN_LOCKED = "unknown-locked"


class Error423Serializer(serializers.Serializer):
Expand Down
29 changes: 25 additions & 4 deletions weblate/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,36 @@

class LockedError(APIException):
status_code = HTTP_423_LOCKED
default_detail = gettext_lazy(
"Could not obtain repository lock to perform the operation."
)
default_code = "repository-locked"
default_detail = gettext_lazy("Could not obtain the lock to perform the operation.")
default_code = "unknown-locked"


class WeblateExceptionHandler(ExceptionHandler):
def convert_known_exceptions(self, exc: Exception) -> Exception:
if isinstance(exc, WeblateLockTimeoutError):
if exc.lock.scope == "repo":
return LockedError(
code="repository-locked",
detail=gettext(
"Could not obtain the repository lock for %s to perform the operation."
)
% exc.lock.origin,
)
if exc.lock.scope == "component-update":
return LockedError(
code="component-locked",
detail=gettext(
"Could not obtain the update lock for component %s to perform the operation."
)
% exc.lock.origin,
)
if exc.lock.origin:
return LockedError(
detail=gettext(
"Could not obtain the %(scope)s lock for %(origin)s to perform the operation."
)
% {"scope": exc.lock.scope, "origin": exc.lock.origin},
)
return LockedError()
return super().convert_known_exceptions(exc)

Expand Down
8 changes: 4 additions & 4 deletions weblate/screenshots/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,10 @@ def ensure_tesseract_language(lang: str) -> None:
# Operate with a lock held to avoid concurrent downloads
with (
WeblateLock(
data_dir("home"),
"screenshots:tesseract-download",
0,
"screenshots:tesseract-download",
lock_path=data_dir("home"),
scope="screenshots:tesseract-download",
key=0,
slug="screenshots:tesseract-download",
timeout=600,
),
sentry_sdk.start_span(op="ocr.models"),
Expand Down
1 change: 1 addition & 0 deletions weblate/trans/models/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,7 @@ def lock(self):
cache_template="{scope}-lock-{key}",
file_template="{slug}-update.lock",
timeout=5,
origin=self.full_slug,
)

@cached_property
Expand Down
8 changes: 7 additions & 1 deletion weblate/utils/backup.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ def ensure_backup_dir():
def backup_lock():
backup_dir = ensure_backup_dir()
return WeblateLock(
backup_dir, "backuplock", 0, "", "lock:{scope}", ".{scope}", timeout=120
lock_path=backup_dir,
scope="backuplock",
key=0,
slug="",
cache_template="lock:{scope}",
file_template=".{scope}",
timeout=120,
)


Expand Down
42 changes: 35 additions & 7 deletions weblate/utils/lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,32 +16,41 @@
from weblate.utils.cache import is_redis_cache

if TYPE_CHECKING:
from types import TracebackType

from django_redis.cache import RedisCache


class WeblateLockTimeoutError(Exception):
"""Weblate lock timeout."""

def __init__(self, message: str, *, lock: WeblateLock) -> None:
super().__init__(message)
self.lock = lock


class WeblateLock:
"""Wrapper around Redis or file based lock."""

def __init__(
self,
*,
lock_path: str,
scope: str,
key: int | str,
slug: str,
cache_template: str = "lock:{scope}:{key}",
file_template: str = "{slug}-{scope}.lock",
timeout: int = 1,
origin: str | None = None,
) -> None:
self._timeout = timeout
self._lock_path = lock_path
self._scope = scope
self._key = key
self._slug = slug
self._depth = 0
self._origin = origin
if is_redis_cache():
# Prefer Redis locking as it works distributed
self._name = self._format_template(cache_template)
Expand All @@ -57,38 +66,57 @@ def __init__(
self._lock = FileLock(self._name, timeout=self._timeout)
self._enter_implementation = self._enter_file

def _format_template(self, template: str):
@property
def scope(self) -> str:
return self._scope

@property
def origin(self) -> str | None:
return self._origin

def _format_template(self, template: str) -> str:
return template.format(
scope=self._scope,
key=self._key,
slug=self._slug,
)

def get_error_message(self) -> str:
if self.origin:
return f"Lock on {self.origin} ({self.scope}) could not be acquired in {self._timeout}s"
return f"Lock on {self._name} could not be acquired in {self._timeout}s"

def _enter_redis(self) -> None:
try:
lock_result = self._lock.acquire(timeout=self._timeout)
except AlreadyAcquired:
return

if not lock_result:
msg = f"Lock on {self._name} could not be acquired in {self._timeout}s"
raise WeblateLockTimeoutError(msg)
raise WeblateLockTimeoutError(self.get_error_message(), lock=self)

def _enter_file(self) -> None:
# Fall back to file based locking
try:
self._lock.acquire()
except Timeout as error:
raise WeblateLockTimeoutError(str(error)) from error
raise WeblateLockTimeoutError(
self.get_error_message(), lock=self
) from error

def __enter__(self):
def __enter__(self) -> None:
self._depth += 1
if self._depth > 1:
return
with sentry_sdk.start_span(op="lock.wait", name=self._name):
self._enter_implementation()

def __exit__(self, exc_type, exc_value, traceback):
def __exit__(
self,
exc_type: type[BaseException] | None,
exc_value: BaseException | None,
traceback: TracebackType | None,
) -> None:
self._depth -= 1
if self._depth > 0:
return
Expand All @@ -98,5 +126,5 @@ def __exit__(self, exc_type, exc_value, traceback):
self._lock.release()

@property
def is_locked(self):
def is_locked(self) -> bool:
return bool(self._depth)
8 changes: 7 additions & 1 deletion weblate/vcs/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,13 @@ def post_migrate(self, sender: AppConfig, **kwargs) -> None:
# We need to do this behind lock to avoid errors when servers
# start in parallel
lockfile = WeblateLock(
home, "gitlock", 0, "", "lock:{scope}", "{scope}", timeout=120
lock_path=home,
scope="gitlock",
key=0,
slug="",
cache_template="lock:{scope}",
file_template="{scope}",
timeout=120,
)
with lockfile:
try:
Expand Down
1 change: 1 addition & 0 deletions weblate/vcs/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def __init__(
slug=os.path.basename(base_path),
file_template="{slug}.lock",
timeout=120,
origin=component.full_slug if component else base_path,
)
self._config_updated = False
self.local = local
Expand Down
8 changes: 4 additions & 4 deletions weblate/vcs/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -1152,10 +1152,10 @@ def request(
self.log(f"HTTP {method} {url}")
cache_id = self.request_time_cache_key
lock = WeblateLock(
data_dir("home"),
f"vcs:api:{vcs_id}",
0,
vcs_id,
lock_path=data_dir("home"),
scope=f"vcs:api:{vcs_id}",
key=0,
slug=vcs_id,
timeout=3 * max(settings.VCS_API_DELAY, 10),
)
try:
Expand Down

0 comments on commit fd3c542

Please sign in to comment.