diff --git a/python/rustwright/sync_api.py b/python/rustwright/sync_api.py index ccc7102..f4f7330 100644 --- a/python/rustwright/sync_api.py +++ b/python/rustwright/sync_api.py @@ -4607,6 +4607,19 @@ def _sleep_until_next_poll(deadline: float, interval: float = 0.02) -> None: time.sleep(min(interval, remaining)) +def _actionability_probe_timeout(remaining_ms: float, timeout_disabled: bool = False) -> float: + # Budget a single actionability/state probe with the full remaining action time rather + # than a small fixed cap. A cap shorter than one remote-CDP round trip made every probe + # time out and abort the action while the outer deadline was still far away (observed on + # click/select_option over connect_over_cdp attach). Polling cadence is driven by + # _sleep_until_next_poll, not this bound, so widening it does not busy-loop. For an + # infinite wait (timeout<=0) keep a bounded cadence so owner-availability and locator + # handlers are still re-checked periodically. + if timeout_disabled: + return min(remaining_ms, 1_000.0) + return max(remaining_ms, 1.0) + + def _handle_event_wait_timeout(owner: Any, event: str, timeout_display: str, deadline: Optional[float]) -> None: if owner is not None: _raise_if_owner_unavailable(owner, use_close_reason=True) @@ -21785,16 +21798,31 @@ def _wait_for_single( while True: _raise_if_owner_unavailable(self._page, method=method) remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0) - command_timeout = min(1_000.0 if timeout_disabled else max(timeout_ms, 1.0), remaining_ms, 1_000.0) + command_timeout = _actionability_probe_timeout(remaining_ms, timeout_disabled) self._page._run_locator_handlers(deadline) - last_info = self._target_state( - command_timeout, - scroll=scroll or state in {"actionable", "scrollable"}, - stable=requires_stable, - receives_events=state == "actionable", - stable_position_required=stable_position_required, - action_position=action_position, - ) + try: + last_info = self._target_state( + command_timeout, + scroll=scroll or state in {"actionable", "scrollable"}, + stable=requires_stable, + receives_events=state == "actionable", + stable_position_required=stable_position_required, + action_position=action_position, + ) + except TimeoutError: + # A single actionability probe that exceeds its own budget is transient, + # not fatal: over a high-latency remote-CDP transport one probe's round + # trips can outlast a small fixed cap. Surface owner unavailability + # immediately, otherwise keep polling until the outer deadline instead of + # aborting the whole action (matches Playwright and the fill/select-apply + # loops). Doing this before the deadline check means a page/context/browser + # that closed mid-probe raises the precise owner error rather than a + # generic timeout when the deadline coincides. + _raise_if_owner_unavailable(self._page, method=method) + if time.monotonic() >= deadline: + break + _sleep_until_next_poll(deadline) + continue if self._strict and not self._explicit_index: self._raise_frame_strict_violation(last_info.get("frame_strict_violation")) count = int(last_info.get("count") or 0) @@ -21857,9 +21885,22 @@ def _wait_for_fill_ready(self, action: str, *, timeout: Optional[float] = None) while True: _raise_if_owner_unavailable(self._page, method=method) remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0) - command_timeout = min(max(timeout_ms, 1.0), remaining_ms, 1_000.0) + command_timeout = _actionability_probe_timeout(remaining_ms) self._page._run_locator_handlers(deadline) - last_info = self._target_state(command_timeout) + try: + last_info = self._target_state(command_timeout) + except TimeoutError: + # A single fill-readiness probe timing out is transient over a slow + # remote-CDP transport: surface owner unavailability immediately, otherwise + # keep polling until the outer deadline (matches the fill/select-apply + # loops). Checking the owner before the deadline break means a closed + # page/context/browser raises the precise owner error rather than a generic + # timeout when the deadline coincides. + _raise_if_owner_unavailable(self._page, method=method) + if time.monotonic() >= deadline: + break + _sleep_until_next_poll(deadline) + continue if self._strict and not self._explicit_index: self._raise_frame_strict_violation(last_info.get("frame_strict_violation")) count = int(last_info.get("count") or 0) @@ -23614,10 +23655,22 @@ def _fill(self, value: str, *, action: str, timeout: Optional[float] = None, for last_info: dict[str, Any] = {} while True: remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0) - command_timeout = min(max(timeout_ms, 1.0), remaining_ms, 1_000.0) + command_timeout = _actionability_probe_timeout(remaining_ms) self._page._run_locator_handlers(deadline) try: result = self._eval(fill_script, command_timeout) + except TimeoutError: + # A single fill-apply probe timing out is transient over a slow remote-CDP + # transport: one probe's CDP round trips can outlast the per-probe budget. + # Surface owner unavailability immediately, otherwise keep polling until the + # outer deadline instead of aborting the whole action (matches Playwright and + # the sibling wait/select loops). TimeoutError subclasses Error, so this must + # precede the generic Error handler below. + _raise_if_owner_unavailable(self._page, method=_locator_method_for_action(action)) + if time.monotonic() >= deadline: + break + _sleep_until_next_poll(deadline) + continue except Error as exc: if "No element matches locator" not in str(exc): raise @@ -24397,10 +24450,13 @@ def _select_option_impl( deadline = started + max(timeout_ms, 0.0) / 1000 while True: remaining_ms = max((deadline - time.monotonic()) * 1000, 1.0) - command_timeout = min(max(timeout_ms, 1.0), remaining_ms, 1_000.0) + command_timeout = _actionability_probe_timeout(remaining_ms) try: result = self._eval(select_script, command_timeout, method=method) except TimeoutError: + # A transient select-apply probe timeout is retried until the outer deadline + # (see the fill-apply loop), but owner unavailability is surfaced immediately. + _raise_if_owner_unavailable(self._page, method=method) if time.monotonic() >= deadline: break _sleep_until_next_poll(deadline) diff --git a/src/lib.rs b/src/lib.rs index fdc2509..46b1583 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9046,12 +9046,18 @@ return true; #[pyo3(signature = (locator_json, index, body, timeout_ms=None))] fn locator_eval( &self, + py: Python<'_>, locator_json: &str, index: usize, body: &str, timeout_ms: Option, ) -> PyResult { - self.evaluate_locator(locator_json, index, body, timeout_ms) + // Release the GIL for the duration of the blocking CDP round-trip, matching + // `click` and `locator_eval_handle`. `evaluate_locator` already detaches inside + // `block_on`, but keeping the detach explicit at the binding layer keeps the + // three locator bindings consistent and guards against a future refactor that + // adds GIL-holding work before `block_on` or changes the transport. + py.detach(|| self.evaluate_locator(locator_json, index, body, timeout_ms)) .map_err(py_err) } diff --git a/tests/test_rustwright_sync_api.py b/tests/test_rustwright_sync_api.py index 940d1b8..d6cc041 100644 --- a/tests/test_rustwright_sync_api.py +++ b/tests/test_rustwright_sync_api.py @@ -10177,6 +10177,167 @@ def test_locator_click_waits_for_delayed_element(page): assert page.evaluate("document.body.dataset.clicked") == "yes" +def test_wait_for_single_retries_transient_actionability_probe_timeout(page, monkeypatch): + # A single actionability probe can exceed its own budget over a high-latency remote-CDP + # transport (connect_over_cdp attach): its several CDP round trips outlast the per-probe + # budget even though the outer action deadline is still far off. The wait loop must treat + # that probe TimeoutError as transient and keep polling, not abort the whole action. + from rustwright.sync_api import Locator + + page.set_content("") + + real_target_state = Locator._target_state + probe = {"calls": 0, "timed_out": 0} + + def flaky_target_state(self, timeout=None, **kwargs): + probe["calls"] += 1 + # Time out only the first probe regardless of how large its budget is, so the retry + # path itself is exercised rather than merely the enlarged per-probe budget. + if probe["timed_out"] == 0: + probe["timed_out"] += 1 + raise TimeoutError(f"timed out after {int(timeout)} ms") + return real_target_state(self, timeout, **kwargs) + + monkeypatch.setattr(Locator, "_target_state", flaky_target_state) + + page.click("#go", timeout=30_000) + + assert page.evaluate("document.body.dataset.clicked") == "yes" + assert probe["timed_out"] == 1 + assert probe["calls"] >= 2 # retried after the transient probe timeout + + +def test_fill_apply_loop_retries_transient_probe_timeout(page, monkeypatch): + # The fill-apply loop performs the actual fill via _eval. A first probe whose CDP round + # trips outlast the per-probe budget raises TimeoutError; the loop must retry until the + # outer deadline instead of re-raising that transient timeout as a fatal error. + from rustwright.sync_api import Locator + + page.set_content("") + + real_eval = Locator._eval + probe = {"calls": 0, "timed_out": 0} + + def flaky_eval(self, body, timeout=None, *, method=None): + if "nonFillableInputTypes" in body: + probe["calls"] += 1 + if probe["timed_out"] == 0: + probe["timed_out"] += 1 + raise TimeoutError(f"timed out after {int(timeout)} ms") + return real_eval(self, body, timeout, method=method) + + monkeypatch.setattr(Locator, "_eval", flaky_eval) + + page.fill("#name", "Ada", timeout=30_000) + + assert page.locator("#name").input_value() == "Ada" + assert probe["timed_out"] == 1 + assert probe["calls"] >= 2 # retried after the transient probe timeout + + +def test_select_apply_loop_retries_transient_probe_timeout(page, monkeypatch): + # The select-apply loop performs the final selection via _eval. A first transient probe + # timeout must be retried until the outer deadline rather than aborting select_option. + from rustwright.sync_api import Locator + + page.set_content( + "" + ) + + real_eval = Locator._eval + probe = {"calls": 0, "timed_out": 0} + + def flaky_eval(self, body, timeout=None, *, method=None): + if "foundValues" in body: + probe["calls"] += 1 + if probe["timed_out"] == 0: + probe["timed_out"] += 1 + raise TimeoutError(f"timed out after {int(timeout)} ms") + return real_eval(self, body, timeout, method=method) + + monkeypatch.setattr(Locator, "_eval", flaky_eval) + + assert page.select_option("#sel", "b", timeout=30_000) == ["b"] + assert probe["timed_out"] == 1 + assert probe["calls"] >= 2 # retried after the transient probe timeout + + +def test_fill_apply_loop_surfaces_outer_deadline_on_persistent_probe_timeout(page, monkeypatch): + # When every fill-apply probe keeps timing out, the loop must still end at the outer + # deadline with its own editable-timeout message (not the raw per-probe timeout) and + # without busy-looping past the deadline. + from rustwright.sync_api import Locator + + page.set_content("") + + real_eval = Locator._eval + + def always_timeout_eval(self, body, timeout=None, *, method=None): + if "nonFillableInputTypes" in body: + raise TimeoutError(f"timed out after {int(timeout)} ms") + return real_eval(self, body, timeout, method=method) + + monkeypatch.setattr(Locator, "_eval", always_timeout_eval) + + started = time.monotonic() + with pytest.raises(TimeoutError, match="editable"): + page.fill("#name", "Ada", timeout=300) + elapsed = time.monotonic() - started + + assert elapsed < 5 # honored the ~300ms outer deadline; no runaway busy-loop + + +def test_fill_apply_loop_propagates_owner_crash_on_probe_timeout(page, monkeypatch): + # A probe timeout must not mask owner unavailability: if the page crashes while a fill + # probe is timing out, the loop surfaces the crash immediately instead of polling until + # the outer deadline. + from rustwright.sync_api import Locator + + page.set_content("") + + real_eval = Locator._eval + + def crashing_probe_eval(self, body, timeout=None, *, method=None): + if "nonFillableInputTypes" in body: + self._page._crashed = True + raise TimeoutError(f"timed out after {int(timeout)} ms") + return real_eval(self, body, timeout, method=method) + + monkeypatch.setattr(Locator, "_eval", crashing_probe_eval) + + try: + with pytest.raises(Error, match="Page crashed"): + page.fill("#name", "Ada", timeout=30_000) + finally: + page._crashed = False + + +def test_wait_for_single_propagates_owner_crash_when_probe_times_out_at_deadline(page, monkeypatch): + # Owner unavailability must win over a probe timeout even when the timeout coincides + # with the outer deadline. The top-of-loop owner check already covers the retry path, + # but a probe that both crashes the owner and outlasts the deadline would otherwise + # take the deadline-break path and raise a generic actionability timeout. The except + # block now surfaces the crash first, matching the fill/select-apply loops. + from rustwright.sync_api import Locator + + page.set_content("") + + def crashing_probe(self, timeout=None, **kwargs): + # Crash the owner and sleep past the tiny outer deadline before timing out, so the + # loop reaches the deadline break rather than re-polling on the next iteration. + self._page._crashed = True + time.sleep(0.05) + raise TimeoutError(f"timed out after {int(timeout)} ms") + + monkeypatch.setattr(Locator, "_target_state", crashing_probe) + + try: + with pytest.raises(Error, match="Page crashed"): + page.click("#go", timeout=10) + finally: + page._crashed = False + + def test_locator_fill_waits_for_delayed_editable(page): page.set_content( """ @@ -23962,6 +24123,62 @@ async def run() -> None: asyncio.run(run()) +def test_locator_eval_does_not_monopolize_gil_while_blocking(page): + # Invariant guard for the PR #95 GIL finding: while native ``locator_eval`` blocks + # on a slow in-page evaluate, it must not hold the Python GIL, so other Python + # threads keep running. This holds because ``locator_eval`` runs the CDP round-trip + # under ``py.detach`` (matching ``click`` / ``locator_eval_handle``); ``block_on`` + # also detaches internally, so the property predates and survives the binding-layer + # change -- this test locks it in against future refactors that could hold the GIL. + # + # The probe is a pure-Python busy-increment thread (no I/O -- ``time.sleep`` would + # release the GIL regardless). If ``locator_eval`` ever held the GIL for the whole + # blocking window, the busy thread would be starved to ~0 iterations, because native + # Rust code never reaches a bytecode boundary where CPython could rotate threads. + # Absolute iteration counts are very noisy (10-20x run-to-run), so the assertion + # only checks "not frozen" with a wide margin rather than a tight rate. + page.set_content("
hi
") + + stop = threading.Event() + progress = {"count": 0} + + def spin() -> None: + while not stop.is_set(): + progress["count"] += 1 + + worker = threading.Thread(target=spin, daemon=True) + worker.start() + try: + # Let the busy thread reach steady state before measuring. + time.sleep(0.05) + before = progress["count"] + started = time.monotonic() + # ``_eval`` routes straight through the native ``locator_eval`` binding. The JS + # body busy-waits ~750ms so the native call blocks for a measurable window. + page.locator("#target")._eval( + "const end = Date.now() + 750; while (Date.now() < end) {} return true;" + ) + elapsed = time.monotonic() - started + advanced = progress["count"] - before + finally: + stop.set() + worker.join(timeout=2) + + # Confirm the in-page busy loop actually blocked the native call (~0.75s), so the + # measurement window is real rather than a fast return. + assert elapsed >= 0.4, ( + f"native locator_eval returned too fast to prove blocking: {elapsed:.3f}s" + ) + # A frozen GIL would starve the busy thread to ~0 iterations. A released GIL lets it + # accumulate hundreds of thousands to millions; 50k is far above 0 yet far below the + # smallest observed released count, so it discriminates "frozen" from "released" + # without depending on the machine's exact throughput. + assert advanced > 50_000, ( + f"busy thread only advanced {advanced} iterations during a {elapsed:.3f}s " + "native locator_eval; the GIL appears to be held for the blocking call" + ) + + def test_connect_over_cdp_cancelled_creations_cleanup_after_last_browser_owner_drops(): async def run() -> None: from playwright.async_api import async_playwright