Skip to content

Commit ca601f1

Browse files
committed
test: test-server fallback without session locks
get_davclient()'s test-server fallback called claim_server() on every server, but the caldav.testing and config-file servers that a pip-installed caldav uses have none, so every server was skipped. A server that failed to start also kept its claim until the process exited. Without fcntl (Windows), embedded and external servers now start unlocked instead of failing; docker servers still need fcntl. Prompt: dismiss the false positive and merge 776. Leave 768 for a while [the false positive: a CodeQL "file is not always closed" warning on a test fixture; 776 is #776 - its merge review found the bugs fixed here; 768 is #768, unrelated] Followup-Prompt: do the needful [fix the bug the merge review found - the fallback skipping every server of a pip-installed caldav - plus the claim left held after a failed start and the missing fcntl on Windows] Assisted-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>
1 parent 8f5199f commit ca601f1

3 files changed

Lines changed: 64 additions & 3 deletions

File tree

‎caldav/config.py‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -413,12 +413,17 @@ def _get_test_server_config(
413413

414414
for server in _collect_test_servers(name, config_file):
415415
was_already_started = server._started
416+
# Servers from caldav.testing (pip-installed caldav) have no session lock
417+
claim = getattr(server, "claim_server", None)
418+
claimed = False
416419
try:
417420
# Another process using this server: try the next one rather than wait
418-
server.claim_server(server.name, 0)
421+
claimed = bool(claim and claim(server.name, 0))
419422
server.start()
420423
except Exception as e:
421424
logging.warning("Failed to start test server %s: %s", server.name, e)
425+
if claimed:
426+
server.release_server(server.name)
422427
continue
423428
logging.info("Using test server: %s at %s", server.name, server.url)
424429
return _test_server_to_params(server, was_already_started)

‎tests/test_servers/base.py‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,16 @@ def claim_server(name: str, timeout: float | None = None) -> bool:
144144
Returns True if the lock was taken just now, False if this process
145145
already held it. Anything learned about the server before a fresh
146146
claim may be stale, since its previous user may have stopped it.
147+
148+
Without ``fcntl`` (Windows) nothing is locked between processes.
147149
"""
148-
import fcntl
149150
import time
150151

152+
try:
153+
import fcntl
154+
except ImportError:
155+
fcntl = None # type: ignore[assignment]
156+
151157
with _session_locks_mutex:
152158
if name in _session_locks:
153159
return False
@@ -163,7 +169,7 @@ def claim_server(name: str, timeout: float | None = None) -> bool:
163169
try:
164170
deadline = time.monotonic() + timeout
165171
announced = False
166-
while True:
172+
while fcntl:
167173
try:
168174
fcntl.flock(lock_file, fcntl.LOCK_EX | fcntl.LOCK_NB)
169175
break
@@ -191,6 +197,14 @@ def claim_server(name: str, timeout: float | None = None) -> bool:
191197
_session_locks[name] = lock_file
192198
return True
193199

200+
@staticmethod
201+
def release_server(name: str) -> None:
202+
"""Give up a claim taken by ``claim_server()``, letting other runs have the server."""
203+
with _session_locks_mutex:
204+
lock_file = _session_locks.pop(name, None)
205+
if lock_file:
206+
lock_file.close()
207+
194208
def claim(self) -> bool:
195209
"""``claim_server()`` for this server."""
196210
return self.claim_server(self.name)

‎tests/test_servers/test_base.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,48 @@ def test_get_davclient_falls_through_busy_server(
227227
assert time.monotonic() - t0 < 5
228228
assert params is not None and "Free" in params["url"]
229229

230+
def test_fallback_uses_servers_without_session_lock(
231+
self, monkeypatch: pytest.MonkeyPatch
232+
) -> None:
233+
"""Pip-installed caldav has no tests/ and so no claim_server() on its servers."""
234+
from caldav import config
235+
236+
server = config._ConfiguredServer("Lambda", {"url": "http://localhost:1/Lambda/"})
237+
monkeypatch.setattr(config, "_collect_test_servers", lambda name, config_file: [server])
238+
params = config._get_test_server_config(None, environment=False)
239+
assert params is not None and "Lambda" in params["url"]
240+
241+
def test_fallback_releases_server_that_failed_to_start(
242+
self, monkeypatch: pytest.MonkeyPatch
243+
) -> None:
244+
import fcntl
245+
246+
from caldav import config
247+
248+
class BrokenServer(base.ExternalTestServer):
249+
def start(self) -> None:
250+
self.claim()
251+
raise RuntimeError("does not start")
252+
253+
broken = BrokenServer({"name": "Mu", "url": "http://localhost:1/Mu/"})
254+
free = base.ExternalTestServer({"name": "Nu", "url": "http://localhost:1/Nu/"})
255+
monkeypatch.setattr(
256+
config, "_collect_test_servers", lambda name, config_file: [broken, free]
257+
)
258+
params = config._get_test_server_config(None, environment=False)
259+
assert params is not None and "Nu" in params["url"]
260+
assert "Mu" not in base._session_locks
261+
with open(base._lock_path("Mu", ".session"), "a+") as other:
262+
fcntl.flock(other, fcntl.LOCK_EX | fcntl.LOCK_NB)
263+
264+
def test_claim_without_fcntl(self, monkeypatch: pytest.MonkeyPatch) -> None:
265+
"""No fcntl (Windows): no lock between processes, but claims still work."""
266+
import sys
267+
268+
monkeypatch.setitem(sys.modules, "fcntl", None)
269+
assert base.TestServer.claim_server("Xi") is True
270+
assert base.TestServer.claim_server("Xi") is False
271+
230272
def test_waiting_run_restarts_container_stopped_meanwhile(self, tmp_path: Path) -> None:
231273
"""Collection saw the container up, but its owner stopped it before we got the lock."""
232274
counter = tmp_path / "runs"

0 commit comments

Comments
 (0)