Repository navigation
fix(gt-i18n): keep translations loaded after the cache expires #36
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| pypi/gt-i18n: patch | ||
| --- | ||
|
|
||
| fix: keep translations loaded after the 60 s cache expiry (serve the cached copy while one background reload runs, keep it when a reload fails), default the CDN URL to cdn.gtx.dev and add the pinned `version_id` and `branch_id` to CDN requests, treat an empty `cache_url` as no CDN loading, and require generaltranslation 0.1.0 and generaltranslation-supported-locales 0.0.6 or later |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,11 @@ | |
|
|
||
| import asyncio | ||
| import inspect | ||
| import threading | ||
| import time | ||
| from collections.abc import Awaitable, Callable | ||
| from concurrent.futures import ThreadPoolExecutor | ||
| from typing import Any | ||
|
|
||
| TranslationsLoader = Callable[[str], dict[str, str] | Awaitable[dict[str, str]]] | ||
|
|
||
|
|
@@ -18,8 +21,30 @@ def __init__(self, translations: dict[str, str], loaded_at: float) -> None: | |
| self.loaded_at = loaded_at | ||
|
|
||
|
|
||
| async def _await(awaitable: Awaitable[Any]) -> Any: | ||
| return await awaitable | ||
|
|
||
|
|
||
| def _run_blocking(awaitable: Awaitable[Any]) -> Any: | ||
| """Run *awaitable* to completion from synchronous code.""" | ||
| try: | ||
| asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| return asyncio.run(_await(awaitable)) | ||
| # A loop is already running in this thread, so run it on a fresh one in another thread. | ||
| with ThreadPoolExecutor(max_workers=1) as pool: | ||
| return pool.submit(asyncio.run, _await(awaitable)).result() | ||
|
|
||
|
|
||
| class TranslationsManager: | ||
| """Caches ``dict[str, str]`` (hash -> translated string) per locale.""" | ||
| """Caches ``dict[str, str]`` (hash -> translated string) per locale. | ||
|
|
||
| An entry expires ``cache_expiry_time`` ms after it loads. An expired entry | ||
| keeps being served while one reload runs in the background, and a reload | ||
| that fails keeps the previous translations, so a slow or failing loader | ||
| never drops a loaded locale back to the source text. Only a locale's first | ||
| load makes its callers wait. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
|
|
@@ -29,47 +54,134 @@ def __init__( | |
| self._loader = loader | ||
| self._cache_expiry_ms = cache_expiry_time | ||
| self._cache: dict[str, _CacheEntry] = {} | ||
| self._loading: dict[str, asyncio.Task[dict[str, str]]] = {} | ||
| self._loading: dict[str, asyncio.Task[None]] = {} | ||
| self._locks: dict[str, threading.Lock] = {} | ||
| self._locks_guard = threading.Lock() | ||
|
|
||
| def _is_expired(self, entry: _CacheEntry) -> bool: | ||
| elapsed_ms = (time.monotonic() - entry.loaded_at) * 1000 | ||
| return elapsed_ms > self._cache_expiry_ms | ||
|
|
||
| async def _call_loader(self, locale: str) -> dict[str, str]: | ||
| result = self._loader(locale) | ||
| if inspect.isawaitable(result): | ||
| return await result | ||
| return result # type: ignore[return-value] | ||
|
|
||
| async def get_translations(self, locale: str) -> dict[str, str]: | ||
| """Load translations for *locale*, using cache when valid.""" | ||
| def _fresh(self, locale: str) -> dict[str, str] | None: | ||
| entry = self._cache.get(locale) | ||
| if entry is not None and not self._is_expired(entry): | ||
| return entry.translations | ||
| return None | ||
|
|
||
| # Deduplicate concurrent loads | ||
| if locale in self._loading: | ||
| return await self._loading[locale] | ||
| def _store(self, locale: str, translations: dict[str, str] | None) -> dict[str, str]: | ||
| """Cache a load result; ``None`` is a failed load, which keeps the previous translations.""" | ||
| if translations is None: | ||
| previous = self._cache.get(locale) | ||
| translations = previous.translations if previous is not None else {} | ||
| self._cache[locale] = _CacheEntry(translations, time.monotonic()) | ||
| return translations | ||
|
|
||
| task = asyncio.ensure_future(self._call_loader(locale)) | ||
| self._loading[locale] = task | ||
| # Async path (FastAPI and other asyncio code) | ||
|
|
||
| async def _load_async(self, locale: str) -> dict[str, str] | None: | ||
| try: | ||
| translations = await task | ||
| if inspect.iscoroutinefunction(self._loader): | ||
| return await self._loader(locale) | ||
| # A synchronous loader runs on a worker thread, off the event loop. | ||
| result = await asyncio.to_thread(self._loader, locale) | ||
| return await result if inspect.isawaitable(result) else result | ||
| except Exception: | ||
| translations = {} | ||
| finally: | ||
| self._loading.pop(locale, None) | ||
| return None | ||
|
|
||
| self._cache[locale] = _CacheEntry(translations, time.monotonic()) | ||
| return translations | ||
| async def _load_and_store(self, locale: str) -> None: | ||
| self._store(locale, await self._load_async(locale)) | ||
|
|
||
| def get_translations_sync(self, locale: str) -> dict[str, str]: | ||
| """Return cached translations or empty dict (never blocks).""" | ||
| def _start_load(self, locale: str) -> asyncio.Task[None]: | ||
| """Start a load for *locale* on the running loop, or join the one already running there.""" | ||
| loop = asyncio.get_running_loop() | ||
| task = self._loading.get(locale) | ||
| if task is None or task.done() or task.get_loop() is not loop: | ||
| task = loop.create_task(self._load_and_store(locale)) | ||
| self._loading[locale] = task | ||
| task.add_done_callback(lambda done: self._forget_load(locale, done)) | ||
| return task | ||
|
|
||
| def _forget_load(self, locale: str, task: asyncio.Task[None]) -> None: | ||
| if self._loading.get(locale) is task: | ||
| del self._loading[locale] | ||
|
|
||
| async def get_translations(self, locale: str) -> dict[str, str]: | ||
| """Return translations for *locale*, loading them if missing and reloading in the background if expired.""" | ||
| fresh = self._fresh(locale) | ||
| if fresh is not None: | ||
| return fresh | ||
| entry = self._cache.get(locale) | ||
| if entry is not None and not self._is_expired(entry): | ||
| if entry is not None: | ||
| # The reload runs on a thread, so it outlives this call's event loop. | ||
| self._refresh_in_background(locale) | ||
| return entry.translations | ||
| return {} | ||
| # Shielded, so a cancelled caller does not cancel the load for the others. | ||
| await asyncio.shield(self._start_load(locale)) | ||
| return self.get_translations_sync(locale) | ||
|
|
||
| async def load_all(self, locales: list[str]) -> None: | ||
| """Eagerly fetch translations for all *locales*.""" | ||
| await asyncio.gather(*(self.get_translations(loc) for loc in locales)) | ||
| """Eagerly fetch translations for all *locales*, waiting for every load.""" | ||
| tasks = [self._start_load(loc) for loc in locales if self._fresh(loc) is None] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If Make both paths share the same in-flight load. Knowledge Base Used: Translation manager lifecycle Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/gt-i18n/src/gt_i18n/i18n_manager/_translations_manager.py
Line: 124
Comment:
**Reloads overwrite newer translations**
If `load_all_translations()` runs while a request reads an expired locale, `_start_load` starts one load and `_refresh_in_background` starts another. They use separate controls for the same locale. Both write the cache without checking which result is newer, so a slower response can overwrite translations already returned by the other load.
Make both paths share the same in-flight load.
**Knowledge Base Used:** [Translation manager lifecycle](https://app.greptile.com/generaltranslation/-/custom-context/knowledge-base/generaltranslation/gt-python/-/docs/translation-manager-lifecycle.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| if tasks: | ||
| await asyncio.gather(*(asyncio.shield(task) for task in tasks)) | ||
|
|
||
| # Blocking path (Flask and other synchronous code) | ||
|
|
||
| def _lock_for(self, locale: str) -> threading.Lock: | ||
| with self._locks_guard: | ||
| return self._locks.setdefault(locale, threading.Lock()) | ||
|
|
||
| def _load_blocking(self, locale: str) -> dict[str, str] | None: | ||
| try: | ||
| result = self._loader(locale) | ||
| return _run_blocking(result) if inspect.isawaitable(result) else result | ||
| except Exception: | ||
| return None | ||
|
|
||
| def _reload_in_background(self, locale: str, lock: threading.Lock) -> None: | ||
| try: | ||
| self._store(locale, self._load_blocking(locale)) | ||
| finally: | ||
| lock.release() | ||
|
|
||
| def _refresh_in_background(self, locale: str) -> None: | ||
| """Reload an expired *locale* on a daemon thread, unless a reload is already running.""" | ||
| lock = self._lock_for(locale) | ||
| if not lock.acquire(blocking=False): | ||
| return | ||
| if self._fresh(locale) is not None: # another reload finished in the meantime | ||
| lock.release() | ||
| return | ||
| try: | ||
| threading.Thread(target=self._reload_in_background, args=(locale, lock), daemon=True).start() | ||
| except BaseException: | ||
| lock.release() | ||
| raise | ||
|
|
||
| def get_translations_blocking(self, locale: str) -> dict[str, str]: | ||
| """Return translations for *locale* from synchronous code. | ||
|
|
||
| The first load blocks the callers for that locale. After that, an | ||
| expired entry is returned at once while one background thread reloads it. | ||
| """ | ||
| fresh = self._fresh(locale) | ||
| if fresh is not None: | ||
| return fresh | ||
| entry = self._cache.get(locale) | ||
| if entry is not None: | ||
| self._refresh_in_background(locale) | ||
| return entry.translations | ||
| with self._lock_for(locale): | ||
| entry = self._cache.get(locale) | ||
| if entry is not None: | ||
| return entry.translations | ||
| return self._store(locale, self._load_blocking(locale)) | ||
|
|
||
| def load_all_blocking(self, locales: list[str]) -> None: | ||
| """Eagerly fetch translations for all *locales* from synchronous code.""" | ||
| _run_blocking(self.load_all(locales)) | ||
|
|
||
| def get_translations_sync(self, locale: str) -> dict[str, str]: | ||
| """Return the last loaded translations, even if expired, or an empty dict (never blocks).""" | ||
| entry = self._cache.get(locale) | ||
| return entry.translations if entry is not None else {} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expired reads run custom async loaders on a new event loop in a daemon thread. If a loader reuses an async client or another object tied to the application's loop, its first load can succeed while every refresh fails.
_load_blockinghides the error, and_storekeeps renewing the old map, so translations never update.Keep async loaders on their owning loop, with a separate path for short-lived loops.
Knowledge Base Used: Translation manager lifecycle
Prompt To Fix With AI