chore: upgrade websockets dependency to support 15+ and update import statements #612
Conversation
WalkthroughRefactors websocket import/connection handling to support websockets v15+ and older versions, centralizes post-connection logic into a helper, relaxes pyproject websockets constraints to permit 15.x on newer Python ranges, and introduces minor CI and test changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Transport as WebSocketTransport
participant WSLib as websockets lib
participant Task as ws_read_loop
Note over Transport,WSLib: import resolution (v15+ top-level or v14- fallback)
Transport->>WSLib: ws_connect(url, headers / additional_headers)
WSLib-->>Transport: websocket
Transport->>Transport: _handle_websocket_connection(ws_url, websocket)
Transport->>Task: create task(ws_read_loop)
Task-->>Transport: on_protocol_message / on_ws_connect_done
alt WebSocketException
Transport->>Transport: dispose() & deactivate() (if not disposed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyproject.toml (1)
43-43: Optional: tighten the 3.8 pin back to <14.0 for clarity.websockets 14.0+ require Python ≥3.9. While your current constraint (>=12,<15) won’t actually install 14.x on 3.8, restoring “<14.0” documents intent and avoids resolver churn. Low-impact, but improves readability. (websockets.readthedocs.io)
Proposed tweak:
- { version = ">= 12.0, < 15.0", python = "~3.8" }, + { version = ">= 12.0, < 14.0", python = "~3.8" },ably/transport/websockettransport.py (1)
14-14: Prefer explicit exception imports; avoid convenience re-exportsThis change ensures that static analyzers and future versions of
websocketscorrectly resolve exceptions fromwebsockets.exceptions, while keepingconnectat top-level (stable since v14.0). We’ve verified this works under both v12.0 and v15.0.1 ofwebsockets.File:
ably/transport/websockettransport.py
Line: 14Minimal import refactor:
-from websockets import WebSocketClientProtocol, connect as ws_connect, ConnectionClosedOK, WebSocketException +from websockets import connect as ws_connect +from websockets.exceptions import ConnectionClosedOK, WebSocketExceptionOptional: avoid a runtime dependency on the legacy
WebSocketClientProtocolby leveragingfrom __future__ import annotationsand aTYPE_CHECKINGblock:if TYPE_CHECKING: try: from websockets.asyncio.client import ClientConnection as WSClient # ≥14.2 except ImportError: try: from websockets.legacy.client import WebSocketClientProtocol as WSClient # ≤13.x except ImportError: from typing import Any as WSClient # then update your annotation: self.websocket: WSClient | None = None✅ Verified under websockets v12.0 and v15.0.1:
connectremains at top-levelConnectionClosedOK&WebSocketExceptionlive inwebsockets.exceptions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
ably/transport/websockettransport.py(1 hunks)pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.8)
- GitHub Check: check (3.11)
- GitHub Check: check (3.7)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
🔇 Additional comments (2)
pyproject.toml (2)
44-44: Align constraint with the stated goal: require websockets ≥ 15.0.1 (not 15.0.0).The PR objective says “allow websockets ≥15.0.1”. Today 15.0.1 is the latest in the 15.x line (released March 5, 2025), and 15.0.0 had a small bugfix follow-up; let’s codify that floor. (websockets.readthedocs.io, pypi.org)
Apply this minimal diff:
- { version = ">= 15.0, < 16.0", python = "^3.9" }, + { version = ">= 15.0.1, < 16.0", python = "^3.9" },
41-45: Compatibility matrix looks good overall.
- 3.7 → websockets 10–11 (ok; 12+ require 3.8+).
- 3.8 → websockets 12–13/13.1 (or 14 excluded per optional nit).
- 3.9+ → websockets 15.x (15 is last to support 3.9; 16 requires 3.10+). (websockets.readthedocs.io)
|
CI is also red on main for all python versions, need to check what's going on 🤔 |
ttypic
force-pushed
the
ECO-5526/upgrade-websockets
branch
from
c209612 to
0318ed4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ably/transport/websockettransport.py (2)
82-89: Header-compat shim is correct; fix the linter error and consider a tiny cleanup.
- The “try
additional_headers→ fallback toextra_headers” sequence matches the API change (extra_headers → additional_headers). (websockets.readthedocs.io)- Lint: GitHub Actions flagged W291 “trailing whitespace” on Line 82. Remove the trailing spaces at the end of the comment.
Apply:
- # Use additional_headers for websockets 15+, fallback to extra_headers for older versions + # Use additional_headers for websockets 15+, fallback to extra_headers for older versions
90-95: Preserve the original traceback cause when re-raising (Ruff B904).Raise the wrapped
AblyExceptionwith the original exception as the cause for clearer diagnostics and to satisfy B904.except (WebSocketException, socket.gaierror) as e: exception = AblyException(f'Error opening websocket connection: {e}', 400, 40000) log.exception(f'WebSocketTransport.ws_connect(): Error opening websocket connection: {exception}') self._emit('failed', exception) - raise exception + raise exception from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
ably/transport/websockettransport.py(2 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
ably/transport/websockettransport.py (4)
ably/util/exceptions.py (1)
AblyException(9-84)ably/util/eventemitter.py (1)
_emit(183-185)ably/types/options.py (1)
loop(226-227)ably/realtime/connectionmanager.py (1)
deactivate_transport(244-246)
🪛 Ruff (0.12.2)
ably/transport/websockettransport.py
94-94: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 GitHub Actions: Linting check
ably/transport/websockettransport.py
[error] 82-82: Flake8: W291 trailing whitespace detected on line 82. Command: 'poetry run flake8'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
- GitHub Check: check (3.7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.10)
🔇 Additional comments (1)
ably/transport/websockettransport.py (1)
14-20: Compat imports verified; no residual deprecated references found
- A repository-wide search for
websockets.client,WebSocketClientProtocol, andextra_headers=returned only the occurrences withinably/transport/websockettransport.py, confirming there are no lingering imports or deprecated API uses elsewhere.- The existing try/except block properly confines the compatibility logic to this file, and no additional clean-up is required.
| async def _handle_websocket_connection(self, ws_url, websocket): | ||
| log.info(f'ws_connect(): connection established to {ws_url}') | ||
| self._emit('connected') | ||
| self.websocket = websocket | ||
| self.read_loop = self.connection_manager.options.loop.create_task(self.ws_read_loop()) | ||
| self.read_loop.add_done_callback(self.on_read_loop_done) | ||
| try: | ||
| await self.read_loop | ||
| except WebSocketException as err: | ||
| if not self.is_disposed: | ||
| await self.dispose() | ||
| self.connection_manager.deactivate_transport(err) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Two fixes: emit order to avoid race, and pass AblyException to deactivate_transport.
- Emit order: You emit “connected” before setting
self.websocket. If any listener sends immediately on the “connected” event,send()will seeself.websocket is Noneand raise. Setself.websocketfirst, then emit. - Reason type:
ConnectionManager.deactivate_transport()expects anOptional[AblyException](see type hints); passing a rawWebSocketExceptionmay break downstream handling. Wrap withAblyException.from_exception(err).
Apply:
async def _handle_websocket_connection(self, ws_url, websocket):
log.info(f'ws_connect(): connection established to {ws_url}')
- self._emit('connected')
- self.websocket = websocket
+ self.websocket = websocket
+ self._emit('connected')
self.read_loop = self.connection_manager.options.loop.create_task(self.ws_read_loop())
self.read_loop.add_done_callback(self.on_read_loop_done)
try:
await self.read_loop
except WebSocketException as err:
if not self.is_disposed:
await self.dispose()
- self.connection_manager.deactivate_transport(err)
+ self.connection_manager.deactivate_transport(AblyException.from_exception(err))If you want to be extra sure we don’t have other call sites passing non-Ably exceptions into deactivate_transport, run:
🏁 Script executed:
#!/usr/bin/env bash
# Inspect deactivate_transport call sites and show the argument expressions.
rg -nP --type=py -C2 '\bdeactivate_transport\s*\('Length of output: 1116
Swap websocket assignment and emit, and wrap exceptions before deactivating transport
Please apply the following mandatory changes in ably/transport/websockettransport.py:
• In _handle_websocket_connection (around lines 96–108), set self.websocket before emitting “connected”, and wrap the caught WebSocketException in an AblyException:
async def _handle_websocket_connection(self, ws_url, websocket):
log.info(f'ws_connect(): connection established to {ws_url}')
- self._emit('connected')
- self.websocket = websocket
+ self.websocket = websocket
+ self._emit('connected')
self.read_loop = self.connection_manager.options.loop.create_task(self.ws_read_loop())
self.read_loop.add_done_callback(self.on_read_loop_done)
try:
await self.read_loop
except WebSocketException as err:
if not self.is_disposed:
await self.dispose()
- self.connection_manager.deactivate_transport(err)
+ self.connection_manager.deactivate_transport(
+ AblyException.from_exception(err)
+ )• There is one other call site at your disconnect(self, reason=None) method (around lines 232–234) that does:
await self.dispose()
self.connection_manager.deactivate_transport(reason)Since deactivate_transport expects an Optional[AblyException], ensure that any non-AblyException value passed in (e.g. a raw exception) is likewise wrapped via AblyException.from_exception(reason) or validate callers only pass AblyException.
🤖 Prompt for AI Agents
In ably/transport/websockettransport.py around lines 96 to 108, the websocket is
assigned after emitting 'connected' and a raw WebSocketException is passed into
deactivate_transport; swap the order so self.websocket = websocket happens
before self._emit('connected'), and in the except WebSocketException block wrap
the caught err in an AblyException (e.g. AblyException.from_exception(err))
before calling self.connection_manager.deactivate_transport; also update the
other call site in disconnect (around lines 232–234) to ensure any
non-AblyException reason is wrapped with AblyException.from_exception(reason)
(or validate callers) before passing to connection_manager.deactivate_transport.
ttypic
force-pushed
the
ECO-5526/upgrade-websockets
branch
from
0318ed4 to
202e0e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ably/transport/websockettransport.py (2)
232-234: Ensure deactivate_transport receives an AblyException (or None)disconnect() may be called with arbitrary exceptions. Wrap non-Ably exceptions for type consistency and downstream handling.
- async def disconnect(self, reason=None): + async def disconnect(self, reason=None): await self.dispose() - self.connection_manager.deactivate_transport(reason) + self.connection_manager.deactivate_transport( + reason if isinstance(reason, AblyException) else AblyException.from_exception(reason) + )Quick scan for other call sites passing non-Ably exceptions:
#!/usr/bin/env bash # Inspect deactivate_transport call sites and show argument expressions and context. rg -nP -C3 --type=py '\bdeactivate_transport\s*\('
96-108: Fix emit order and exception type passed to deactivate_transportEmit “connected” only after assigning self.websocket to avoid send() races; and pass an AblyException into deactivate_transport (it expects Optional[AblyException]).
async def _handle_websocket_connection(self, ws_url, websocket): log.info(f'ws_connect(): connection established to {ws_url}') - self._emit('connected') - self.websocket = websocket + self.websocket = websocket + self._emit('connected') self.read_loop = self.connection_manager.options.loop.create_task(self.ws_read_loop()) self.read_loop.add_done_callback(self.on_read_loop_done) try: await self.read_loop except WebSocketException as err: if not self.is_disposed: await self.dispose() - self.connection_manager.deactivate_transport(err) + self.connection_manager.deactivate_transport( + AblyException.from_exception(err) + )
🧹 Nitpick comments (7)
test/ably/rest/restchannelpublish_test.py (1)
406-408: Replace fixed sleeps with polling to reduce flakiness and test timeHard-coded sleeps make the suite slow and still flaky under variable load. Poll until the condition is satisfied or a short timeout elapses.
- # temporary added delay, we need to investigate why messages don't appear immediately - await asyncio.sleep(1) + # Wait for message visibility with bounded polling to avoid flakiness + for _ in range(10): # ~1s worst case + async with httpx.AsyncClient(http2=True) as client: + r = await client.get(url, auth=auth) + if r.is_success and r.json(): + break + await asyncio.sleep(0.1) ... - # temporary added delay, we need to investigate why messages don't appear immediately - await asyncio.sleep(1) + # Wait for history to reflect the message (bounded polling) + for _ in range(10): + history = await channel.history() + if history.items: + break + await asyncio.sleep(0.1)If you prefer a reusable helper instead of inlined loops, I can add a local async utility (kept in this test file) to poll with a timeout—say, wait_for(predicate, timeout=1.5, interval=0.1). Want me to push that as a follow-up?
Also applies to: 418-421
ably/transport/websockettransport.py (6)
90-95: Preserve original exception context when re-raisingUse exception chaining to aid debugging and satisfy linters (ruff B904).
- except (WebSocketException, socket.gaierror) as e: + except (WebSocketException, socket.gaierror) as e: exception = AblyException(f'Error opening websocket connection: {e}', 400, 40000) log.exception(f'WebSocketTransport.ws_connect(): Error opening websocket connection: {exception}') self._emit('failed', exception) - raise exception + raise exception from e
61-68: Create tasks on the configured loop for consistencyconnect() uses asyncio.create_task while ws_read_loop uses self.connection_manager.options.loop.create_task. Mixing loops can lead to subtle lifecycle issues if a non-default loop is configured.
- self.ws_connect_task = asyncio.create_task(self.ws_connect(ws_url, headers)) + self.ws_connect_task = self.connection_manager.options.loop.create_task( + self.ws_connect(ws_url, headers) + )
205-206: Avoid raising a bare Exception when websocket is not setRaise a typed exception to aid upstream handling and diagnostics.
- if self.websocket is None: - raise Exception() + if self.websocket is None: + raise AblyException('send() called with no active websocket', 500, 50000)
171-177: Typo in callback name (protcol) — consider renaming for clarityMinor but noisy for maintenance and searchability.
- task.add_done_callback(self.on_protcol_message_handled) + task.add_done_callback(self.on_protocol_message_handled) ... - def on_protcol_message_handled(self, task): + def on_protocol_message_handled(self, task):Note: Adjust all references accordingly.
Also applies to: 167-168
80-90: Header-arg fallback is good; optionally pre-detect to avoid try/except cost on every connectYour TypeError fallback is pragmatic. If you want to avoid relying on exceptions for flow control, detect the parameter name once via inspect.signature and choose the appropriate kwarg. Not critical.
Example (outside hot path, e.g., module import):
import inspect _SIG = inspect.signature(ws_connect) _HAS_ADDITIONAL = 'additional_headers' in _SIG.parametersThen:
kwargs = {'additional_headers' if _HAS_ADDITIONAL else 'extra_headers': headers} async with ws_connect(ws_url, **kwargs) as websocket: await self._handle_websocket_connection(ws_url, websocket)
14-20: Prefer top-levelwebsockets.connectand use a typing-only import for the protocolWe’ve confirmed that in websockets v15,
websockets.connectremains available at the top level and supports theadditional_headersparameter, and- there’s no official need to import a separate client type at runtime.
You can simplify the import logic to only pull in
connect, falling back for older versions, and defer theWebSocketClientProtocolimport to type‐checking time:try: # Top-level connect is stable in v15+ - from websockets import ClientConnection as WebSocketClientProtocol, connect as ws_connect + from websockets import connect as ws_connect except ImportError: # Fallback for websockets ≤14 - from websockets.client import WebSocketClientProtocol, connect as ws_connect + from websockets.client import connect as ws_connect # type: ignore[assignment] +from typing import TYPE_CHECKING +if TYPE_CHECKING: + # Only for type annotations; no runtime dependency on websockets internals + from websockets.client import WebSocketClientProtocol +else: + WebSocketClientProtocol = object # placeholder at runtime• This eliminates version-sensitive imports at module load, relying only on
connect(stable across versions).
• UseTYPE_CHECKINGso your runtime bundle isn’t tied to the internal protocol class.
• You can still annotate functions/methods that return or acceptWebSocketClientProtocol.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/check.yml(1 hunks)ably/transport/websockettransport.py(2 hunks)pyproject.toml(1 hunks)test/ably/rest/restchannelpublish_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (2)
test/ably/rest/restchannelpublish_test.py (2)
ably/rest/channel.py (2)
get(182-193)publish(106-132)ably/types/message.py (1)
Message(24-226)
ably/transport/websockettransport.py (4)
ably/util/exceptions.py (1)
AblyException(9-84)ably/util/eventemitter.py (1)
_emit(183-185)ably/types/options.py (1)
loop(226-227)ably/realtime/connectionmanager.py (1)
deactivate_transport(244-246)
🪛 Ruff (0.12.2)
ably/transport/websockettransport.py
94-94: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
.github/workflows/check.yml (2)
39-39: Pytest verbosity change is fineThis will help triage the currently red CI with clearer, shorter tracebacks. No concerns.
21-21: Matrix includes Python 3.13; manual verification of actions/Poetry support requiredThe GitHub CLI dispatch script failed with “HTTP 403: Resource not accessible by integration,” so we can’t automatically confirm that
setup-python@v2/abatilo/actions-poetry@v2.0.0with Poetry 1.3.2 will pull in proper Python 3.13 runners on ubuntu-22.04. To avoid CI breakage unrelated to this PR’s intent, please:• Bump your actions versions and Poetry, then either
– Manually trigger a workflow run for Python 3.13 (via the GitHub UI) and inspect the logs, or
– Temporarily exclude 3.13 from the matrix until support is confirmed.• Suggested safe-modernization tweaks:
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 ... - - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v5 ... - - name: Setup poetry - uses: abatilo/actions-poetry@v2.0.0 + - name: Setup poetry + uses: abatilo/actions-poetry@v2 with: - poetry-version: 1.3.2 + poetry-version: 1.7.1• To gate 3.13 until you’ve confirmed support, adjust:
- python-version: ['3.7','3.8','3.9','3.10','3.11','3.12','3.13'] + python-version: ['3.7','3.8','3.9','3.10','3.11','3.12']Please verify the upgraded actions support 3.13 in your environment before re-adding it to the matrix.
| if encoding == 'json': | ||
| assert json.loads(item['data']) == json.loads(data) | ||
| assert json.loads(item['data']) == json.loads(msg_data) | ||
| else: | ||
| assert item['data'] == data | ||
| assert item['data'] == msg_data | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle composite encodings (e.g., 'json/utf-8') instead of only 'json'
The test assumes an exact 'json' encoding. Wire encodings can be composite (e.g., 'json/utf-8'). Use a prefix or token check to avoid false negatives.
- if encoding == 'json':
- assert json.loads(item['data']) == json.loads(msg_data)
+ if encoding.startswith('json'):
+ assert json.loads(item['data']) == json.loads(msg_data)
else:
assert item['data'] == msg_data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if encoding == 'json': | |
| assert json.loads(item['data']) == json.loads(data) | |
| assert json.loads(item['data']) == json.loads(msg_data) | |
| else: | |
| assert item['data'] == data | |
| assert item['data'] == msg_data | |
| if encoding.startswith('json'): | |
| assert json.loads(item['data']) == json.loads(msg_data) | |
| else: | |
| assert item['data'] == msg_data |
🤖 Prompt for AI Agents
In test/ably/rest/restchannelpublish_test.py around lines 412 to 416, the test
currently checks encoding == 'json' which fails for composite encodings like
'json/utf-8'; update the check to inspect the media type prefix (e.g.,
encoding.split('/', 1)[0].lower() == 'json' or
encoding.lower().startswith('json/')) and treat any such composite encoding as
JSON so the assertion uses json.loads for those cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ably/transport/websockettransport.py (1)
96-108: Fix race: assign websocket before emitting 'connected'; and pass AblyException to deactivate_transportEmitting before assignment can trigger send() on a None websocket if listeners send immediately. Also, deactivate_transport expects an Optional[AblyException]; passing WebSocketException may break downstream expectations. This mirrors a prior review comment—still applies.
Apply:
- log.info(f'ws_connect(): connection established to {ws_url}') - self._emit('connected') - self.websocket = websocket + log.info(f'ws_connect(): connection established to {ws_url}') + self.websocket = websocket + self._emit('connected') @@ - except WebSocketException as err: + except WebSocketException as err: if not self.is_disposed: await self.dispose() - self.connection_manager.deactivate_transport(err) + self.connection_manager.deactivate_transport( + AblyException.from_exception(err) + )Follow-up: Review other call sites of deactivate_transport to ensure they pass AblyException consistently (e.g., disconnect(reason)).
Run this to inspect call sites and arguments:
#!/usr/bin/env bash rg -nP --type=py -C2 '\bdeactivate_transport\s*\('
🧹 Nitpick comments (3)
test/ably/rest/restchannelpublish_test.py (1)
12-12: Import asyncio: OK, but keep it local to the test if only used thereImport is only needed for the sleeps introduced in test_interoperability. Keeping it at module level is fine; alternatively, import inside the test to emphasize scope.
ably/transport/websockettransport.py (2)
14-20: Prefer canonical import paths to reduce aliasing ambiguity across versionsTop-level aliases exist in newer websockets (ClientConnection since 14.2; connect since 14.0), but relying on them can be brittle. Import from websockets.asyncio.client when available, and fall back to legacy/older paths only if needed. This avoids surprises if aliases change in future majors. According to upstream docs, additional_headers is the asyncio argument in 15+, while extra_headers is used in older APIs. (websockets.readthedocs.io)
Apply:
-try: - # websockets 15+ preferred imports - from websockets import ClientConnection as WebSocketClientProtocol, connect as ws_connect -except ImportError: - # websockets 14 and earlier fallback - from websockets.client import WebSocketClientProtocol, connect as ws_connect +try: + # websockets 14.0+ (new asyncio implementation / canonical path) + from websockets.asyncio.client import ClientConnection as WebSocketClientProtocol, connect as ws_connect +except Exception: + # websockets 13.x and earlier (legacy / older paths) + from websockets.client import WebSocketClientProtocol, connect as ws_connectIf you want to support 14.x where aliases exist, this remains compatible while preferring the definitive module.
91-95: Preserve exception chaining when re-raisingRaise the wrapped AblyException with cause to improve diagnostics and satisfy Ruff B904.
Apply:
- except (WebSocketException, socket.gaierror) as e: + except (WebSocketException, socket.gaierror) as e: exception = AblyException(f'Error opening websocket connection: {e}', 400, 40000) log.exception(f'WebSocketTransport.ws_connect(): Error opening websocket connection: {exception}') self._emit('failed', exception) - raise exception + raise exception from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/check.yml(1 hunks)ably/transport/websockettransport.py(2 hunks)pyproject.toml(1 hunks)test/ably/rest/restchannelpublish_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
ably/transport/websockettransport.py (4)
ably/util/exceptions.py (1)
AblyException(9-84)ably/util/eventemitter.py (1)
_emit(183-185)ably/types/options.py (1)
loop(226-227)ably/realtime/connectionmanager.py (1)
deactivate_transport(244-246)
🪛 Ruff (0.12.2)
ably/transport/websockettransport.py
94-94: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (5)
.github/workflows/check.yml (1)
39-39: Pytest verbosity tweak looks good; helps triage CI failures fasterAdding --verbose with --tb=short improves signal-to-noise in logs without flooding CI. No functional risk.
test/ably/rest/restchannelpublish_test.py (3)
394-396: Variable rename to msg_data improves clarity and avoids shadowingGood rename; avoids confusion with the outer data variable from the JSON file and matches its usage in later assertions.
413-416: Assertion changes: confirm binary case expectations from fixtureSwitching to json.loads(msg_data) for encoding == 'json' and comparing item['data'] to msg_data otherwise looks correct. Please confirm messages-encoding.json provides msg_data in the server’s wire format for non-JSON types (e.g., base64 for binary), otherwise this may mask encoding mismatches.
Would you like a small helper to normalize msg_data per encoding before assertion?
418-419: Publishing with explicit Message(data=msg_data, encoding=encoding) is clearerThis matches the preceding assertions and keeps the test self-consistent. Good change.
ably/transport/websockettransport.py (1)
82-90: Header compatibility shim is correct for 15+ vs older versionsTrying additional_headers first and falling back to extra_headers is aligned with v15 docs and earlier versions. This guards the handshake across versions. (websockets.readthedocs.io)
| # temporary added delay, we need to investigate why messages don't appear immediately | ||
| await asyncio.sleep(1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed sleeps with bounded polling to deflake the test
Hard sleeps introduce flakiness and add unnecessary latency. Prefer polling until the condition holds (with a timeout) to accommodate eventual consistency without slowing the happy path.
Apply this refactor within the loop:
- # temporary added delay, we need to investigate why messages don't appear immediately
- await asyncio.sleep(1)
+ # Wait (up to 5s) for message to appear to reduce flakiness
+ for _ in range(50):
+ async with httpx.AsyncClient(http2=True) as client:
+ r = await client.get(url, auth=auth)
+ if r.status_code == 200 and r.json():
+ break
+ await asyncio.sleep(0.1)And after the second publish:
- # temporary added delay, we need to investigate why messages don't appear immediately
- await asyncio.sleep(1)
+ # Wait (up to 5s) for message to appear in history
+ for _ in range(50):
+ history = await channel.history()
+ if history.items:
+ break
+ await asyncio.sleep(0.1)If you prefer not to duplicate client creation, hoist a single AsyncClient outside the loop and reuse it. I can propose that variant if you want.
Also applies to: 419-420
🤖 Prompt for AI Agents
In test/ably/rest/restchannelpublish_test.py around lines 406-407 (and similarly
at 419-420), replace the hard-coded asyncio.sleep(1) calls with a bounded
polling loop that repeatedly checks the expected condition until it becomes true
or a timeout is reached; implement a small interval (e.g. 50-200ms) between
checks, raise/assert a failure if the timeout elapses, and reuse a single
AsyncClient created outside the loop (or pass the existing client) to avoid
duplicating client creation — this removes flakiness and unnecessary delay while
preserving a maximum wait time.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/ably/rest/restchannelpublish_test.py (2)
412-415: Handle composite JSON encodings (e.g., 'json/utf-8') instead of only equality with 'json'The check
if encoding == 'json'misses cases likejson/utf-8. Treat any encoding whose primary token isjsonas JSON.Apply:
- if encoding == 'json': + if encoding and encoding.lower().split('/', 1)[0] == 'json': assert json.loads(item['data']) == json.loads(msg_data) else: assert item['data'] == msg_data
406-407: Deflake: replace fixed sleeps with bounded pollingHard sleeps cause flakiness and slow tests. Poll until the message appears, with a short timeout. This also keeps the happy path fast.
A targeted refactor for the two delays:
- # temporary added delay, we need to investigate why messages don't appear immediately - await sleep(1) + # Wait (up to 5s) for message to appear, polling every 100ms + for _ in range(50): + async with httpx.AsyncClient(http2=True) as client: + r = await client.get(url, auth=auth) + if r.status_code == 200 and r.json(): + break + await sleep(0.1)- # temporary added delay, we need to investigate why messages don't appear immediately - await sleep(1) + # Wait (up to 5s) for message to appear in history + for _ in range(50): + history = await channel.history() + if history.items: + break + await sleep(0.1)If you prefer, I can propose a small helper to avoid duplicating the polling loop.
Also applies to: 419-420
🧹 Nitpick comments (2)
test/ably/rest/restchannelpublish_test.py (1)
424-425: Use isinstance for type checksSlightly more idiomatic and resilient to subclasses.
- assert type(message.data) == type_mapping[expected_type] + assert isinstance(message.data, type_mapping[expected_type])ably/scripts/unasync.py (1)
186-223: Optional: support aliasing in the special-case (safe when only 'sleep' is imported)Currently,
from asyncio import sleep as swon’t be rewritten even though it’s safe. Consider acceptingasaftersleepwhile still rejecting any additional import items.Here’s a focused tweak:
- # Check if next is exactly "sleep" - if (check_counter < len(tokens) and tokens[check_counter].src == "sleep"): + # Check if next is exactly "sleep" + if check_counter < len(tokens) and tokens[check_counter].src == "sleep": # Check what comes after "sleep" to determine if this is a standalone sleep import next_check = check_counter + 1 is_sleep_only = True @@ - if next_check < len(tokens): + if next_check < len(tokens): next_token = tokens[next_check] - # If there's a comma, "as", or other import items, it's not a standalone sleep import - if next_token.src in [",", "as"] or next_token.name == "NAME": - is_sleep_only = False + # Allow aliasing with "as", but still reject additional import items (comma or another NAME after alias) + if next_token.src == ",": + is_sleep_only = False + elif next_token.src == "as": + # Allow alias: ensure no additional imports after alias target + alias_check = next_check + 1 + while alias_check < len(tokens) and tokens[alias_check].src == " ": + alias_check += 1 + # If there's a comma or another NAME beyond alias, reject + if alias_check < len(tokens) and tokens[alias_check].src in [","] : + is_sleep_only = False + elif next_token.name == "NAME": + # Another NAME immediately after sleep implies more imports + is_sleep_only = FalseIf you want, I can add a small unit test to lock this behavior in.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ably/scripts/unasync.py(2 hunks)test/ably/rest/restchannelpublish_test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/ably/rest/restchannelpublish_test.py (2)
ably/rest/channel.py (2)
get(182-193)publish(106-132)ably/types/message.py (1)
Message(24-226)
🪛 GitHub Actions: Linting check
ably/scripts/unasync.py
[error] 185-185: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 188-188: flake8 (poetry run flake8): W291 trailing whitespace.
[error] 189-189: flake8 (poetry run flake8): E129 visually indented line with same indent as next logical line.
[error] 190-190: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 193-193: flake8 (poetry run flake8): W291 trailing whitespace.
[error] 207-207: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 211-211: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 217-217: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 222-222: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 227-227: flake8 (poetry run flake8): W293 blank line contains whitespace.
[error] 246-246: flake8 (poetry run flake8): W293 blank line contains whitespace.
test/ably/rest/restchannelpublish_test.py
[error] 420-420: flake8 (poetry run flake8): F821 undefined name 'asyncio'.
🪛 Ruff (0.12.2)
test/ably/rest/restchannelpublish_test.py
420-420: Undefined name asyncio
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check (3.11)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.7)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
- GitHub Check: check (3.8)
🔇 Additional comments (3)
test/ably/rest/restchannelpublish_test.py (2)
394-396: LGTM: local alias clarifies intentUsing
msg_data = input_msg['data']improves readability below, especially where both JSON and non-JSON flows are asserted.
419-420: Use the importedsleepinstead of the undefinedasynciomodule inrestchannelpublish_test.pyThe test file imports
sleep(fromasyncio) but does not import theasynciomodule, soawait asyncio.sleep(1)is undefined and breaks both CI (F821) and the unasync rewrite.• File needing change:
test/ably/rest/restchannelpublish_test.pyat line 420Suggested minimal diff:
- # temporary added delay, we need to investigate why messages don't appear immediately - await asyncio.sleep(1) + # temporary added delay, we need to investigate why messages don't appear immediately + await sleep(1)ably/scripts/unasync.py (1)
185-246: Fix flake8 W291/W293/E129 in ably/scripts/unasync.pyThe following changes remove trailing/blank-line whitespace and simplify the visually indented
ifto a single line, preserving the special-case logic for “from asyncio import sleep” → “from time import sleep”:--- a/ably/scripts/unasync.py +++ b/ably/scripts/unasync.py @@ def some_method(...): - full_lib_name = '' - lib_name_counter = token_counter + 2 - + full_lib_name = '' + lib_name_counter = token_counter + 2 - # Handle special case for "from asyncio import sleep" -> "from time import sleep" - # First check if this is an asyncio import with just sleep - if (lib_name_counter < len(tokens) and - tokens[lib_name_counter].src == "asyncio"): - + # Handle special case for "from asyncio import sleep" -> "from time import sleep" + # First check if this is an asyncio import with just sleep + if lib_name_counter < len(tokens) and tokens[lib_name_counter].src == "asyncio": - # Skip whitespace + # Skip whitespace while check_counter < len(tokens) and tokens[check_counter].src == " ": check_counter += 1 @@ - # Skip whitespace after "import" + # Skip whitespace after "import" while check_counter < len(tokens) and tokens[check_counter].src == " ": check_counter += 1 @@ - # Check what comes after "sleep" to determine if this is a standalone sleep import + # Check what comes after "sleep" to determine if this is a standalone sleep import next_check = check_counter + 1 is_sleep_only = True @@ - + @@ - while lib_name_counter < len(tokens) and tokens[lib_name_counter].src != " ": + while lib_name_counter < len(tokens) and tokens[lib_name_counter].src != " ": full_lib_name += tokens[lib_name_counter].src lib_name_counter += 1 @@ - # For all other cases, add the original module name tokens - for lib_name_part in full_lib_name.split("."): - lib_name_part = self._class_rename(lib_name_part) - new_tokens.append(tokenize_rt.Token("NAME", lib_name_part)) - new_tokens.append(tokenize_rt.Token("OP", ".")) - if full_lib_name: # Only remove the last dot if we added tokens - new_tokens.pop() - + # For all other cases, add the original module name tokens + for lib_name_part in full_lib_name.split("."): + lib_name_part = self._class_rename(lib_name_part) + new_tokens.append(tokenize_rt.Token("NAME", lib_name_part)) + new_tokens.append(tokenize_rt.Token("OP", ".")) + if full_lib_name: # Only remove the last dot if we added tokens + new_tokens.pop() + return lib_name_counter• Removed trailing spaces from blank lines at 185, 190, 207, 211, 217, 222, 227, and 246.
• Consolidated the two-lineif (…and…):into one line to eliminate E129.
• Normalized comment whitespace to eliminate W291/W293.With these adjustments, all flake8 violations (W291/W293/E129) should be resolved without altering runtime behavior.
ttypic
force-pushed
the
ECO-5526/upgrade-websockets
branch
from
b4a3a9d to
202e0e8
Compare
Resolves #606
Summary by CodeRabbit
Refactor
Chores
Tests