Conversation
WalkthroughUpdates library version to v2.1.4, adjusts WebSocket transport to select scheme by TLS, improves idle-timer management and cleanup on normal close, and adds tests (WsProxy + test) validating immediate reconnection after a normal WebSocket close. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant Transport as WebSocket Transport
participant Server as Ably Server
participant Reconnect as Reconnection Handler
Client->>Transport: connect(tls=true or false)
Transport->>Server: establish ws:// or wss:// connection (chosen by TLS)
Server-->>Transport: connection established
Transport-->>Client: connected
Server->>Transport: send normal close (1000)
Transport->>Transport: read loop exits normally
Transport->>Transport: dispose resources and deactivate transport
Transport->>Reconnect: trigger immediate reconnect
Reconnect->>Transport: attempt reconnection
Transport->>Server: establish ws:// or wss:// connection
Server-->>Transport: connection restored
Transport-->>Client: reconnected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
…nnection logic - Added local WebSocket proxy for testing (`WsProxy`) and corresponding tests for immediate reconnection on normal close. - Fixed missing reconnection on server-sent normal WebSocket close frames in `WebSocketTransport`. - Adjusted idle timer handling to avoid accidental reuse. (cherry picked from commit 69a7527)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 7: Replace the inconsistent h3 heading "### What's Changed" with an h2
heading "## What's Changed" so it matches the other changelog entries; locate
the heading string "What's Changed" in CHANGELOG.md and update its markdown
prefix from "###" to "##" for consistency.
In `@test/ably/realtime/realtimeconnection_test.py`:
- Around line 479-485: The test passes an invalid Options key
`endpoint=proxy.endpoint` so the Ably client ignores the proxy and connects to
the real host; change the call to TestApp.get_ably_realtime to pass
realtime_host and port (e.g. realtime_host=proxy.host, port=proxy.port) instead,
and if WsProxy lacks a port property add one or change WsProxy to expose the
proxy port via a `port` attribute so the test can use `port=proxy.port` when
calling TestApp.get_ably_realtime.
- Line 35: The annotation self.port: int | None is Python 3.10+ syntax and will
break older supported versions; change the annotations to use typing.Optional
(e.g. from typing import Optional) or add from __future__ import annotations at
the top of the file; update the related attribute _close_event's annotation
similarly (replace its use of | None with Optional[...] or rely on the future
annotations import) and ensure the corresponding imports (typing.Optional) are
added if you choose that route.
- Around line 4-11: The test imports connect directly causing import errors on
older websockets versions; wrap the import of connect in a try/except in
test/ably/realtime/realtimeconnection_test.py so it first tries "from websockets
import connect as _ws_connect" and on ImportError falls back to "from
websockets.client import connect as _ws_connect", mirroring the pattern used in
ably/transport/websockettransport.py and ensuring the _ws_connect symbol is
available across websockets 14 and 15+.
- Around line 41-42: Replace the incompatible attribute access
client_ws.request.path with the cross-version compatible client_ws.path when
building target_url (e.g., in the code that sets path and target_url); update
the line that reads path = client_ws.request.path to path = client_ws.path so
target_url = f"wss://{self.target_host}{path}" works across websockets 12–15.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2df10b1-3ac5-4866-9f64-c142e55d6ac8
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdably/__init__.pyably/transport/websockettransport.pypyproject.tomltest/ably/realtime/realtimeconnection_test.py
|
|
||
| [Full Changelog](https://github.com/ably/ably-python/compare/v2.1.3...v2.1.4) | ||
|
|
||
| ### What's Changed |
There was a problem hiding this comment.
Minor formatting inconsistency with heading level.
Line 7 uses ### What's Changed (h3), but previous changelog entries (lines 15, 24, 33, etc.) use ## What's Changed (h2). Consider using h2 for consistency with the existing format.
📝 Suggested fix
-### What's Changed
+## What's Changed📝 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.
| ### What's Changed | |
| ## What's Changed |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 7, Replace the inconsistent h3 heading "### What's
Changed" with an h2 heading "## What's Changed" so it matches the other
changelog entries; locate the heading string "What's Changed" in CHANGELOG.md
and update its markdown prefix from "###" to "##" for consistency.
| from websockets import connect as _ws_connect | ||
|
|
||
| try: | ||
| # websockets 15+ preferred import | ||
| from websockets.asyncio.server import serve as ws_serve | ||
| except ImportError: | ||
| # websockets 14 and earlier fallback | ||
| from websockets.server import serve as ws_serve |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how websockets connect is imported in the main codebase
rg -n "from websockets" --type=pyRepository: ably/ably-python
Length of output: 869
🏁 Script executed:
cat -n ably/transport/websockettransport.py | head -30Repository: ably/ably-python
Length of output: 1178
🏁 Script executed:
# Check if _ws_connect is used in the test file
rg -n "_ws_connect" test/ably/realtime/realtimeconnection_test.py
# Check websockets version constraints
rg -n "websockets" pyproject.toml setup.py requirements.txt 2>/dev/null || trueRepository: ably/ably-python
Length of output: 398
Add try/except fallback for websockets.connect import to match version compatibility.
Line 4 imports connect from websockets without a fallback, but websockets changed the import location between versions. For websockets 14 and earlier (used with Python 3.7/3.8), the import must be from websockets.client import connect, while websockets 15+ uses from websockets import connect. The main code in ably/transport/websockettransport.py already handles this with a try/except pattern (lines 17-24). The test file should use the same approach:
try:
# websockets 15+ preferred import
from websockets import connect as _ws_connect
except ImportError:
# websockets 14 and earlier fallback
from websockets.client import connect as _ws_connectWithout this, the test will fail to import on Python 3.7/3.8 with websockets 10-14.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ably/realtime/realtimeconnection_test.py` around lines 4 - 11, The test
imports connect directly causing import errors on older websockets versions;
wrap the import of connect in a try/except in
test/ably/realtime/realtimeconnection_test.py so it first tries "from websockets
import connect as _ws_connect" and on ImportError falls back to "from
websockets.client import connect as _ws_connect", mirroring the pattern used in
ably/transport/websockettransport.py and ensuring the _ws_connect symbol is
available across websockets 14 and 15+.
| def __init__(self, target_host: str): | ||
| self.target_host = target_host | ||
| self.server = None | ||
| self.port: int | None = None |
There was a problem hiding this comment.
Python 3.7-3.9 compatibility issue with union type syntax.
The int | None syntax requires Python 3.10+ without a from __future__ import annotations import. Since the project supports Python 3.7+ (per pyproject.toml), this will cause a SyntaxError on older Python versions.
🐛 Option 1: Add annotations import at file top
from __future__ import annotations🐛 Option 2: Use Optional from typing
+from typing import Optional
+
class WsProxy:
"""Local WS proxy that forwards to real Ably and lets tests trigger a normal close."""
def __init__(self, target_host: str):
self.target_host = target_host
self.server = None
- self.port: int | None = None
+ self.port: Optional[int] = None
self._close_event: asyncio.Event | None = NoneNote: Also update line 36 for _close_event.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ably/realtime/realtimeconnection_test.py` at line 35, The annotation
self.port: int | None is Python 3.10+ syntax and will break older supported
versions; change the annotations to use typing.Optional (e.g. from typing import
Optional) or add from __future__ import annotations at the top of the file;
update the related attribute _close_event's annotation similarly (replace its
use of | None with Optional[...] or rely on the future annotations import) and
ensure the corresponding imports (typing.Optional) are added if you choose that
route.
| path = client_ws.request.path # e.g. "/?key=...&format=json" | ||
| target_url = f"wss://{self.target_host}{path}" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
websockets 15 python ServerConnection request path attribute
💡 Result:
In websockets 15.x (new asyncio API), the server handler no longer receives path as a 2nd argument. Instead, get the request path from the handshake request object:
async def handler(conn):
path = conn.request.path # e.g. "/chat?room=1"
headers = conn.request.headersServerConnection.request is available after the opening handshake and contains the HTTP handshake Request object. In migration terms: connection.path → connection.request.path (and request_headers → request.headers). [1], [2], [3]
🏁 Script executed:
# Find websockets version requirement
git ls-files | grep -E "(requirements|setup|pyproject|poetry)" | head -20Repository: ably/ably-python
Length of output: 84
🏁 Script executed:
# Search for websockets imports and version info in test file
rg -n "websockets|ServerConnection|WebSocketServerProtocol" test/ably/realtime/realtimeconnection_test.pyRepository: ably/ably-python
Length of output: 487
🏁 Script executed:
# Check for version constraints in common config files
find . -maxdepth 2 -type f \( -name "*.txt" -o -name "*.toml" -o -name "*.cfg" -o -name "*.yaml" -o -name "*.yml" \) | xargs grep -l "websockets" 2>/dev/null | head -10Repository: ably/ably-python
Length of output: 76
🏁 Script executed:
# Check websockets version requirement in pyproject.toml
cat pyproject.toml | grep -A 10 -B 2 "websockets"Repository: ably/ably-python
Length of output: 550
🏁 Script executed:
# Read the test file context around lines 41-42 to understand client_ws
sed -n '1,50p' test/ably/realtime/realtimeconnection_test.pyRepository: ably/ably-python
Length of output: 1872
🏁 Script executed:
# Search for other handler usages and websockets API calls in the codebase
rg -n "\.request\." --type py test/ably/realtime/realtimeconnection_test.pyRepository: ably/ably-python
Length of output: 132
🏁 Script executed:
# Check if there are other websockets handlers in the codebase
rg -n "async def.*handler|request\.path|\.path" --type py test/ | head -30Repository: ably/ably-python
Length of output: 1638
🏁 Script executed:
# Search for websockets version checks or conditionals in the codebase
rg -n "websockets|version|__version__" test/ably/realtime/realtimeconnection_test.pyRepository: ably/ably-python
Length of output: 487
🌐 Web query:
websockets 12 13 14 WebSocketServerProtocol path attribute
💡 Result:
In the Python websockets library (v12, v13, v14), the request path is still available, but how you’re expected to access it changed:
WebSocketServerProtocol.pathexists (legacy server protocol object) — you can read it aswebsocket.path. The docs explicitly recommend using the.pathattribute of the first handler argument instead of relying on a separatepathparameter. [1]- The old handler signature
async def handler(websocket, path): ...had its 2ndpathparameter made optional earlier, and in websockets 13.0 it’s deprecated (you should switch towebsocket.path). [2]
If you’re using the new asyncio implementation (introduced and migrated via websockets.asyncio.*), you generally won’t be working with WebSocketServerProtocol at all:
websockets.legacy.server.WebSocketServerProtocol(old)websockets.asyncio.server.ServerConnection(new) [3]
So for the new API, access the path via the connection’s HTTP request (per the migration guidance):
connection.request.path(new implementation), rather thanWebSocketServerProtocol.path. [3]
Sources:
[1] websockets changelog (path available via .path on first handler argument) (websockets.readthedocs.io)
[2] websockets 13.0.1 changelog (2nd path handler parameter deprecated; use request.path) (websockets.readthedocs.io)
[3] websockets 13 upgrade guide (legacy WebSocketServerProtocol vs new ServerConnection) (websockets.readthedocs.io)
Citations:
- 1: https://websockets.readthedocs.io/en/14.0/project/changelog.html
- 2: https://websockets.readthedocs.io/en/13.0.1/project/changelog.html
- 3: https://websockets.readthedocs.io/en/13.0/howto/upgrade.html
🏁 Script executed:
# Check if there's any version detection or compatibility code already in the codebase
rg -n "websockets.__version__|pkg_resources|importlib" --type py . | head -20Repository: ably/ably-python
Length of output: 42
Fix websockets API compatibility: use client_ws.path instead of client_ws.request.path.
The code uses client_ws.request.path, which only exists in websockets 15+ (ServerConnection). For Python 3.8 (supporting websockets 12-14 with WebSocketServerProtocol), the .request attribute does not exist. Use the compatible client_ws.path attribute, which works across all supported versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ably/realtime/realtimeconnection_test.py` around lines 41 - 42, Replace
the incompatible attribute access client_ws.request.path with the cross-version
compatible client_ws.path when building target_url (e.g., in the code that sets
path and target_url); update the line that reads path = client_ws.request.path
to path = client_ws.path so target_url = f"wss://{self.target_host}{path}" works
across websockets 12–15.
| async with WsProxy(self.test_vars["host"]) as proxy: | ||
| ably = await TestApp.get_ably_realtime( | ||
| disconnected_retry_timeout=500_000, | ||
| suspended_retry_timeout=500_000, | ||
| tls=False, | ||
| endpoint=proxy.endpoint, | ||
| ) |
There was a problem hiding this comment.
Test will not use the proxy — endpoint is not a valid option.
The endpoint parameter is not recognized by Options (see ably/types/options.py:27-102). Unknown kwargs are passed through TestApp.get_ably_realtime() but silently ignored by AblyRealtime. The client will connect to the real Ably host instead of the local proxy, defeating the test's purpose.
Use realtime_host and port separately instead.
🐛 Proposed fix
First, update the WsProxy.endpoint property to expose port directly, or update the test to use the port:
ably = await TestApp.get_ably_realtime(
disconnected_retry_timeout=500_000,
suspended_retry_timeout=500_000,
tls=False,
- endpoint=proxy.endpoint,
+ realtime_host="127.0.0.1",
+ port=proxy.port,
)📝 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.
| async with WsProxy(self.test_vars["host"]) as proxy: | |
| ably = await TestApp.get_ably_realtime( | |
| disconnected_retry_timeout=500_000, | |
| suspended_retry_timeout=500_000, | |
| tls=False, | |
| endpoint=proxy.endpoint, | |
| ) | |
| async with WsProxy(self.test_vars["host"]) as proxy: | |
| ably = await TestApp.get_ably_realtime( | |
| disconnected_retry_timeout=500_000, | |
| suspended_retry_timeout=500_000, | |
| tls=False, | |
| realtime_host="127.0.0.1", | |
| port=proxy.port, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ably/realtime/realtimeconnection_test.py` around lines 479 - 485, The
test passes an invalid Options key `endpoint=proxy.endpoint` so the Ably client
ignores the proxy and connects to the real host; change the call to
TestApp.get_ably_realtime to pass realtime_host and port (e.g.
realtime_host=proxy.host, port=proxy.port) instead, and if WsProxy lacks a port
property add one or change WsProxy to expose the proxy port via a `port`
attribute so the test can use `port=proxy.port` when calling
TestApp.get_ably_realtime.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ably/realtime/realtimeconnection_test.py (1)
51-53: Await canceled relay tasks during cleanup.At Line 51-Line 53, tasks are canceled but not awaited. This can leave pending tasks during teardown.
♻️ Proposed fix
finally: c2s.cancel() s2c.cancel() close_task.cancel() + await asyncio.gather(c2s, s2c, close_task, return_exceptions=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ably/realtime/realtimeconnection_test.py` around lines 51 - 53, The teardown cancels relay tasks (c2s, s2c, close_task) but never awaits them, leaving pending tasks; after calling c2s.cancel(), s2c.cancel(), and close_task.cancel() await their completion using asyncio.gather(c2s, s2c, close_task, return_exceptions=True) or await each task in a try/except that suppresses asyncio.CancelledError so all three tasks are properly awaited and any exceptions are handled during cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ably/realtime/realtimeconnection_test.py`:
- Around line 54-55: The test currently swallows all errors with "except
Exception: pass", which hides real proxy/connectivity failures; locate that bare
except in test/ably/realtime/realtimeconnection_test.py (the try/except around
the proxy handler setup) and remove it or change it to catch only the specific
expected exception and re-raise unexpected ones (e.g., "except
ExpectedErrorType: ..." or "except Exception as e: raise") so proxy handler
failures fail the test instead of being ignored.
---
Nitpick comments:
In `@test/ably/realtime/realtimeconnection_test.py`:
- Around line 51-53: The teardown cancels relay tasks (c2s, s2c, close_task) but
never awaits them, leaving pending tasks; after calling c2s.cancel(),
s2c.cancel(), and close_task.cancel() await their completion using
asyncio.gather(c2s, s2c, close_task, return_exceptions=True) or await each task
in a try/except that suppresses asyncio.CancelledError so all three tasks are
properly awaited and any exceptions are handled during cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b92a498-be7b-4d5f-bded-88cc31e8993a
📒 Files selected for processing (2)
CHANGELOG.mdtest/ably/realtime/realtimeconnection_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Do not swallow proxy handler failures.
At Line 54-Line 55, except Exception: pass can mask real proxy/connectivity failures and let this regression test pass for the wrong reason.
🐛 Proposed fix
- except Exception:
- pass
+ except Exception as exc:
+ raise RuntimeError("WsProxy handler failed") from exc📝 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.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| raise RuntimeError("WsProxy handler failed") from exc |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ably/realtime/realtimeconnection_test.py` around lines 54 - 55, The test
currently swallows all errors with "except Exception: pass", which hides real
proxy/connectivity failures; locate that bare except in
test/ably/realtime/realtimeconnection_test.py (the try/except around the proxy
handler setup) and remove it or change it to catch only the specific expected
exception and re-raise unexpected ones (e.g., "except ExpectedErrorType: ..." or
"except Exception as e: raise") so proxy handler failures fail the test instead
of being ignored.
2 participants
Hotfix release for Python v2
Summary by CodeRabbit