From a4edd0f4dbe6692c52ff6b435d4ab7e9c0979c66 Mon Sep 17 00:00:00 2001 From: Blai Peidro Date: Sat, 12 Sep 2026 03:00:33 +0200 Subject: [PATCH] refactor: make the pending-subscription sentinel explicit `WSClient._should_subscribe_to_pending_job` holds `False` until a pending subscription is queued and a dict afterwards, and three places subscript it. The subscripts are safe, because the only path that reaches them tests the flag first, but the test was buried: ```python if all([message.get('group_name') == 'jobs', message.get('status') == 'pending', message.get('unified_job_id'), self._should_subscribe_to_pending_job]): if bool(message.get('project_id')) == (self._should_subscribe_to_pending_job['events'] == 'project_update_events'): self._update_subscription(message['unified_job_id']) ``` Four unrelated conditions in an `all([...])`, one of them the guard for the line below it, and `_update_subscription` then reaching back for the attribute a second time rather than being handed it. Three changes, none of which alters behaviour: - The sentinel is `None` rather than `False`, annotated `dict | None`. Both are falsy and nothing compares it by identity or to `False`, so every existing check behaves the same. `None` is what "not set yet" means. - The flag is bound to a local and tested first, with `and` instead of `all([...])`. Short-circuiting rather than eager evaluation, which is fine here since every element is a pure `.get()`. - `_update_subscription` takes the dict as an argument instead of re-reading the attribute, so it cannot be called in a state where that attribute is unset. Exercised the whole path directly, since the unit suite covers the callbacks but not this branch: queueing with `subscribe_to_pending_events('job_events')`, then feeding a pending-job message through `_on_message`, resubscribes with `{'jobs': ['status_changed'], 'job_events': [7]}` and clears the sentinel back to `None`. Four diagnostics retired. Diffed the full list before and after: strict subset, nothing introduced. Note for whoever merges: #27 also edits this file, so whichever lands second needs a rebase. Verified with `black --check`, `flake8` and the unit suite, 355 passing. --- ascenderkit/ws.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/ascenderkit/ws.py b/ascenderkit/ws.py index 7977bee..7ec3a0a 100644 --- a/ascenderkit/ws.py +++ b/ascenderkit/ws.py @@ -101,7 +101,9 @@ def __init__( url, on_open=self._on_open, on_message=self._on_message, on_error=self._on_error, on_close=self._on_close, cookie=auth_cookie ) self._message_cache = [] - self._should_subscribe_to_pending_job = False + # None rather than False: this holds a dict once a pending subscription is + # queued, and the sentinel only ever has to be falsy. + self._should_subscribe_to_pending_job: dict | None = None self._pending_unsubscribe = threading.Event() self._add_received_time = add_received_time @@ -212,9 +214,12 @@ def _on_message(self, ws, message): if self._add_received_time: message['received_time'] = datetime.now(timezone.utc) - if all([message.get('group_name') == 'jobs', message.get('status') == 'pending', message.get('unified_job_id'), self._should_subscribe_to_pending_job]): - if bool(message.get('project_id')) == (self._should_subscribe_to_pending_job['events'] == 'project_update_events'): - self._update_subscription(message['unified_job_id']) + # Bound once so the rest of the block can rely on it, which all([...]) of + # four unrelated conditions did not make obvious. + pending = self._should_subscribe_to_pending_job + if pending and message.get('group_name') == 'jobs' and message.get('status') == 'pending' and message.get('unified_job_id'): + if bool(message.get('project_id')) == (pending['events'] == 'project_update_events'): + self._update_subscription(pending, message['unified_job_id']) ret = self._recv_queue.put(message) @@ -224,12 +229,12 @@ def _on_message(self, ws, message): return ret - def _update_subscription(self, job_id): - subscription = dict(jobs=self._should_subscribe_to_pending_job['jobs']) - events = self._should_subscribe_to_pending_job['events'] + def _update_subscription(self, pending, job_id): + subscription = dict(jobs=pending['jobs']) + events = pending['events'] subscription[events] = [job_id] self.subscribe(**subscription) - self._should_subscribe_to_pending_job = False + self._should_subscribe_to_pending_job = None def _on_open(self, ws): self._ws_connected_flag.set()