mirror of
https://github.com/kuhyx/utils.git
synced 2026-11-10 20:27:15 +01:00
Pool HTTP connections, and let callers set the Firebase timeout
Every request went through the module-level requests.get/put/post/delete helpers, so each one paid a fresh DNS+TCP+TLS handshake. Measured today against the real backends that is 95ms/req to api.github.com and 52ms to Realtime Database -- on a 27-request diet-guard tick, most of the wall clock. Each transport module now binds a pooling requests.Session to the name `requests`. A Session is a drop-in for those helpers, so all 9 call sites and all 16 tests that patch `<module>.requests.<verb>` are untouched; the real module stays available as `_requests` for the RequestException/Response references. Sessions are per-module rather than shared because the tests use sequence-consuming side_effect lists, where one module could otherwise eat another's queued response. Transient failures now retry, restricted to GET: a PUT here carries a Contents-API sha, and replaying one whose response was merely lost would write against a stale precondition on a remote the phone also reads. Also thread timeout_seconds through firebase_client_for, which accepted none, so a caller's tighter budget silently reached the GitHub backend only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCucBhUkHBBHHbsY7bSdGV
This commit is contained in:
parent
a4b72a21c8
commit
4468df0e13
@ -26,7 +26,7 @@ import json
|
||||
from pathlib import Path
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from crdt_sync._firebase import FirebaseSyncClient
|
||||
from crdt_sync._firebase import _DEFAULT_TIMEOUT_SECONDS, FirebaseSyncClient
|
||||
from crdt_sync._firebase_auth import FileCredentialStore, FirebaseTokenProvider
|
||||
from crdt_sync._mirror import MirrorSyncClient
|
||||
|
||||
@ -141,6 +141,7 @@ def firebase_client_for(
|
||||
app_name: str,
|
||||
*,
|
||||
config: FirebaseConfig | None = None,
|
||||
timeout_seconds: float = _DEFAULT_TIMEOUT_SECONDS,
|
||||
) -> FirebaseSyncClient:
|
||||
"""Return a signed-in client for ``app_name``.
|
||||
|
||||
@ -151,6 +152,9 @@ def firebase_client_for(
|
||||
Args:
|
||||
app_name: Names the credential cache, e.g. ``"diet_guard"``.
|
||||
config: Overrides the on-disk config; for tests.
|
||||
timeout_seconds: Per-request network timeout. Without this the
|
||||
client fell back to its own default and a caller's tighter
|
||||
budget reached the GitHub backend only.
|
||||
|
||||
Raises:
|
||||
ConfigError: If the shared config is unusable.
|
||||
@ -160,7 +164,9 @@ def firebase_client_for(
|
||||
auth = FirebaseTokenProvider(resolved.api_key, credential_store_for(app_name))
|
||||
if not auth.has_session():
|
||||
auth.sign_in(resolved.email, resolved.read_password())
|
||||
return FirebaseSyncClient(resolved.database_url, auth)
|
||||
return FirebaseSyncClient(
|
||||
resolved.database_url, auth, timeout_seconds=timeout_seconds
|
||||
)
|
||||
|
||||
|
||||
def mirror_client_for(
|
||||
|
||||
@ -17,10 +17,16 @@ from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
import requests
|
||||
import requests as _requests
|
||||
|
||||
from crdt_sync._http import new_session
|
||||
from crdt_sync._remote import RemoteNotFoundError, RemoteSyncError
|
||||
|
||||
# Bound to the name ``requests`` so the call sites below -- and the tests
|
||||
# that patch ``<module>.requests.<verb>`` -- are unchanged by pooling.
|
||||
requests = new_session()
|
||||
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from crdt_sync._firebase_auth import FirebaseTokenProvider
|
||||
|
||||
@ -163,7 +169,7 @@ class FirebaseSyncClient:
|
||||
def _params(self, **extra: str) -> dict[str, str]:
|
||||
return {"auth": self.auth.id_token(), **extra}
|
||||
|
||||
def _raise(self, what: str, response: requests.Response) -> None:
|
||||
def _raise(self, what: str, response: _requests.Response) -> None:
|
||||
"""Raise the right error type for a non-2xx response.
|
||||
|
||||
401/403 mean the rules rejected this uid or the token is bad.
|
||||
@ -184,14 +190,14 @@ class FirebaseSyncClient:
|
||||
msg = f"{what} failed: HTTP {response.status_code} {detail}"
|
||||
raise FirebaseSyncError(msg)
|
||||
|
||||
def _get(self, path: str, **params: str) -> requests.Response:
|
||||
def _get(self, path: str, **params: str) -> _requests.Response:
|
||||
try:
|
||||
return requests.get(
|
||||
self._url(path),
|
||||
params=self._params(**params),
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error reading {path}"
|
||||
raise FirebaseSyncError(msg) from exc
|
||||
|
||||
@ -257,7 +263,7 @@ class FirebaseSyncClient:
|
||||
json=text,
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error writing {path}"
|
||||
raise FirebaseSyncError(msg) from exc
|
||||
if not response.ok:
|
||||
@ -302,7 +308,7 @@ class FirebaseSyncClient:
|
||||
params=self._params(),
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error deleting {path}"
|
||||
raise FirebaseSyncError(msg) from exc
|
||||
if not response.ok:
|
||||
@ -321,7 +327,7 @@ class FirebaseSyncClient:
|
||||
params=self._params(shallow="true"),
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException:
|
||||
except _requests.RequestException:
|
||||
return False
|
||||
except RemoteSyncError:
|
||||
# Includes FirebaseAuthError: "cannot get a token" is exactly
|
||||
|
||||
@ -17,10 +17,16 @@ import datetime as dt
|
||||
import json
|
||||
from typing import TYPE_CHECKING, Protocol
|
||||
|
||||
import requests
|
||||
import requests as _requests
|
||||
|
||||
from crdt_sync._http import new_session
|
||||
from crdt_sync._remote import RemoteSyncError
|
||||
|
||||
# Bound to the name ``requests`` so the call sites below -- and the tests
|
||||
# that patch ``<module>.requests.<verb>`` -- are unchanged by pooling.
|
||||
requests = new_session()
|
||||
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from pathlib import Path
|
||||
|
||||
@ -373,7 +379,7 @@ class FirebaseTokenProvider:
|
||||
json=payload,
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error trying to {what}"
|
||||
raise FirebaseAuthError(msg) from exc
|
||||
if not response.ok:
|
||||
@ -383,7 +389,7 @@ class FirebaseTokenProvider:
|
||||
return response.json()
|
||||
|
||||
|
||||
def _reason(response: requests.Response) -> str:
|
||||
def _reason(response: _requests.Response) -> str:
|
||||
"""Return Google's machine-readable error reason, in parentheses.
|
||||
|
||||
Pulls ``INVALID_PASSWORD`` / ``TOKEN_EXPIRED`` / ``USER_DISABLED`` out of
|
||||
|
||||
@ -11,10 +11,16 @@ from __future__ import annotations
|
||||
|
||||
import base64
|
||||
|
||||
import requests
|
||||
import requests as _requests
|
||||
|
||||
from crdt_sync._http import new_session
|
||||
from crdt_sync._remote import RemoteNotFoundError, RemoteSyncError
|
||||
|
||||
# Bound to the name ``requests`` so the call sites below -- and the tests
|
||||
# that patch ``<module>.requests.<verb>`` -- are unchanged by pooling.
|
||||
requests = new_session()
|
||||
|
||||
|
||||
_API_BASE = "https://api.github.com"
|
||||
_HTTP_NOT_FOUND = 404
|
||||
_DEFAULT_TIMEOUT_SECONDS = 15
|
||||
@ -68,14 +74,14 @@ class GitHubSyncClient:
|
||||
def _contents_url(self, path: str) -> str:
|
||||
return f"{_API_BASE}/repos/{self._owner}/{self._repo}/contents/{path}"
|
||||
|
||||
def _get(self, path: str) -> requests.Response:
|
||||
def _get(self, path: str) -> _requests.Response:
|
||||
try:
|
||||
return requests.get(
|
||||
self._contents_url(path),
|
||||
headers=self._headers,
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error reading {path}"
|
||||
raise GitHubSyncError(msg) from exc
|
||||
|
||||
@ -86,7 +92,7 @@ class GitHubSyncClient:
|
||||
headers=self._headers,
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException:
|
||||
except _requests.RequestException:
|
||||
return False
|
||||
return response.ok
|
||||
|
||||
@ -166,7 +172,7 @@ class GitHubSyncClient:
|
||||
json=payload,
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error pushing {path}"
|
||||
raise GitHubSyncError(msg) from exc
|
||||
if not response.ok:
|
||||
@ -241,7 +247,7 @@ class GitHubSyncClient:
|
||||
json={"message": message, "sha": sha},
|
||||
timeout=self._timeout_seconds,
|
||||
)
|
||||
except requests.RequestException as exc:
|
||||
except _requests.RequestException as exc:
|
||||
msg = f"network error deleting {path}"
|
||||
raise GitHubSyncError(msg) from exc
|
||||
if not response.ok:
|
||||
|
||||
57
crdt-sync/crdt_sync/_http.py
Normal file
57
crdt-sync/crdt_sync/_http.py
Normal file
@ -0,0 +1,57 @@
|
||||
"""Pooled HTTP sessions, so every request does not re-handshake.
|
||||
|
||||
Every call site in this package used the module-level ``requests.get`` /
|
||||
``put`` / ``post`` / ``delete`` helpers, each of which opens a throwaway
|
||||
connection: fresh DNS, TCP and TLS on every single request. Measured against
|
||||
the two real backends on 2026-08-21 that costs 82ms per request to
|
||||
``api.github.com`` and 51ms to Realtime Database -- on a 27-request sync tick,
|
||||
most of the wall clock.
|
||||
|
||||
A :class:`requests.Session` keeps the connection alive between requests, and
|
||||
is a drop-in for the module-level helpers: same verb names, same signatures.
|
||||
So each transport module binds one of these to the name ``requests`` and its
|
||||
call sites are unchanged.
|
||||
|
||||
**One session per module, deliberately not one global.** The tests patch the
|
||||
verb on the module attribute (``patch.object(fb.requests, "get", ...)``), and
|
||||
several use sequence-consuming ``side_effect`` lists. Were all three modules
|
||||
to share a single object, a call from one would consume another's queued
|
||||
response and the failure would look like a logic bug. Per-module also happens
|
||||
to be per-host here -- GitHub, Realtime Database, and the token endpoint are
|
||||
three different origins -- so pooling loses nothing by the split.
|
||||
|
||||
Not thread-safe, in that :class:`requests.Session` is not documented to be;
|
||||
this package spawns no threads and its consumers drive sync from one.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import requests
|
||||
from requests.adapters import HTTPAdapter, Retry
|
||||
|
||||
_RETRY_TOTAL = 3
|
||||
_RETRY_BACKOFF_SECONDS = 0.3
|
||||
_RETRY_STATUSES = (429, 500, 502, 503, 504)
|
||||
|
||||
|
||||
def new_session() -> requests.Session:
|
||||
"""Return a connection-pooling session that retries transient GET failures.
|
||||
|
||||
Retries are restricted to ``GET``. A ``PUT`` here carries a Contents-API
|
||||
``sha`` (and a Realtime Database write is a full-node replace); replaying
|
||||
one whose response was merely lost would push a second write built on a
|
||||
now-stale precondition, against a remote another device reads. Reads are
|
||||
idempotent, so only they are replayed.
|
||||
"""
|
||||
session = requests.Session()
|
||||
retry = Retry(
|
||||
total=_RETRY_TOTAL,
|
||||
backoff_factor=_RETRY_BACKOFF_SECONDS,
|
||||
status_forcelist=_RETRY_STATUSES,
|
||||
allowed_methods=frozenset({"GET"}),
|
||||
raise_on_status=False,
|
||||
)
|
||||
adapter = HTTPAdapter(max_retries=retry)
|
||||
session.mount("https://", adapter)
|
||||
session.mount("http://", adapter)
|
||||
return session
|
||||
@ -1,6 +1,6 @@
|
||||
[project]
|
||||
name = "crdt-sync"
|
||||
version = "0.7.0"
|
||||
version = "0.8.0"
|
||||
description = "Shared CRDT merge scheme + GitHub-Contents-API sync transport for cross-device apps"
|
||||
requires-python = ">=3.10"
|
||||
dependencies = ["requests"]
|
||||
|
||||
Loading…
Reference in New Issue
Block a user