Skip to content

Propagate cancellation out of the reconnect loop - #981

Draft
caspervonb wants to merge 1 commit into
mainfrom
propagate-reconnect-cancellation
Draft

Propagate cancellation out of the reconnect loop#981
caspervonb wants to merge 1 commit into
mainfrom
propagate-reconnect-cancellation

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

The reconnect loop's per-server except arm caught asyncio.CancelledError together with TimeoutError and continued to the next server, so a cancellation landing inside establish_connection was swallowed. A close() during an in-progress reconnect would then block on the read task until the entire reconnect schedule (servers × attempts × backoff) ran dry, instead of returning promptly.

Re-raise CancelledError; only TimeoutError is a per-server failure. Covered by a test that drives a reconnect through the public API against an endpoint that goes silent on reconnect, cancels the in-flight loop, and asserts it stops.

The reconnect loop's per-server except arm caught asyncio.CancelledError
alongside TimeoutError and continued to the next server, swallowing the
cancellation. A close() landing while the loop was inside
establish_connection then kept trying servers instead of stopping,
blocking close() on the read task until the whole reconnect schedule ran
dry. Re-raise CancelledError; only TimeoutError is a per-server failure.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 22, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 3m 16s —— View job


PR Review

  • Gather context
  • Review source changes
  • Review test changes
  • Post findings

The fix is correct and the approach is minimal. The source change is small (+4/-2) and the test comprehensively covers the failure mode.

Source change (nats-core/src/nats/client/__init__.py): Splitting except (asyncio.CancelledError, TimeoutError) into two separate handlers is the right pattern. Since Python 3.8+, asyncio.CancelledError inherits from BaseException — not Exception — so the outer except Exception: block wrapping the full reconnect-loop body was already letting cancellation pass through. The only place it was explicitly swallowed was this inner per-server handler. The log message change (type(e).__name__"timeout") is a minor improvement.

Test (nats-core/tests/test_client.py): Well-constructed. Using a real asyncio.start_server instead of mocks means the test exercises the actual establish_connection I/O path (specifically the blocking reader.readline() waiting for INFO) where the bug manifests. The second_connection event prevents cancelling before the reconnect attempt is actually in progress, and the finally block does complete cleanup.

One thing to be aware of (pre-existing, not introduced here): when CancelledError propagates out of the reconnect loop, self._reconnecting is left as True and self._status stays as RECONNECTING — the cleanup lines (_reconnecting = False, _status = ClientStatus.CLOSED) are only reached on the "exhausted all attempts" path. close() recovers correctly since it overrides both, and the test correctly calls await client.close() in the finally block. But bare cancellation without a subsequent close() leaves the client in a stuck state. This is worth documenting or hardening in a follow-up (e.g., a try/finally around the reconnect loop body), but it's out of scope for this PR.

except (asyncio.CancelledError, TimeoutError) as e:
logger.error("Failed to connect to %s: %s", server, type(e).__name__)
except asyncio.CancelledError:
raise

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer except Exception: at the while-loop level (~line 1062) was already safe — asyncio.CancelledError is a BaseException in Python 3.8+ (and nats-core targets 3.13+), so it wouldn't have been caught there. The only place it was explicitly swallowed was this inner per-server handler, which this change correctly fixes.

One side-effect worth noting: when CancelledError propagates out of this block, the cleanup at the bottom of the while loop (self._reconnecting = False, self._status = ClientStatus.CLOSED, lines 1065–1067) is never reached, leaving the client in a stuck RECONNECTING/_reconnecting=True state. close() recovers cleanly from this (it overrides both), and the test guards against it with await client.close() in the finally block — but bare cancellation without a subsequent close() call leaves the client unusable. This is a pre-existing structural concern, not something this PR introduces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant