Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Change Log

## [v2.1.4](https://github.com/ably/ably-python/tree/v2.1.4)

[Full Changelog](https://github.com/ably/ably-python/compare/v2.1.3...v2.1.4)

### What's Changed
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
### 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.


- Fixed handling of normal WebSocket close frames and improved reconnection logic [#672](https://github.com/ably/ably-python/pull/672)

## [v2.1.3](https://github.com/ably/ably-python/tree/v2.1.3)

[Full Changelog](https://github.com/ably/ably-python/compare/v2.1.2...v2.1.3)
Expand Down
2 changes: 1 addition & 1 deletion ably/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@
logger.addHandler(logging.NullHandler())

api_version = '3'
lib_version = '2.1.3'
lib_version = '2.1.4'
13 changes: 10 additions & 3 deletions ably/transport/websockettransport.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ def __init__(self, connection_manager: ConnectionManager, host: str, params: dic
def connect(self):
headers = HttpUtils.default_headers()
query_params = urllib.parse.urlencode(self.params)
ws_url = (f'wss://{self.host}?{query_params}')
scheme = 'wss' if self.options.tls else 'ws'
ws_url = f'{scheme}://{self.host}?{query_params}'
log.info(f'connect(): attempting to connect to {ws_url}')
self.ws_connect_task = asyncio.create_task(self.ws_connect(ws_url, headers))
self.ws_connect_task.add_done_callback(self.on_ws_connect_done)
Expand Down Expand Up @@ -110,6 +111,11 @@ async def _handle_websocket_connection(self, ws_url, websocket):
if not self.is_disposed:
await self.dispose()
self.connection_manager.deactivate_transport(err)
else:
# Read loop exited normally (e.g., server sent normal WS close frame)
if not self.is_disposed:
await self.dispose()
self.connection_manager.deactivate_transport()

async def on_protocol_message(self, msg):
self.on_activity()
Expand Down Expand Up @@ -214,8 +220,9 @@ async def send(self, message: dict):
await self.websocket.send(raw_msg)

def set_idle_timer(self, timeout: float):
if not self.idle_timer:
self.idle_timer = Timer(timeout, self.on_idle_timer_expire)
if self.idle_timer:
self.idle_timer.cancel()
self.idle_timer = Timer(timeout, self.on_idle_timer_expire)

async def on_idle_timer_expire(self):
self.idle_timer = None
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "ably"
version = "2.1.3"
version = "2.1.4"
description = "Python REST and Realtime client library SDK for Ably realtime messaging service"
readme = "LONG_DESCRIPTION.rst"
requires-python = ">=3.7"
Expand Down
104 changes: 104 additions & 0 deletions test/ably/realtime/realtimeconnection_test.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
import asyncio

import pytest
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
Comment on lines +4 to +11
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how websockets connect is imported in the main codebase
rg -n "from websockets" --type=py

Repository: ably/ably-python

Length of output: 869


🏁 Script executed:

cat -n ably/transport/websockettransport.py | head -30

Repository: 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 || true

Repository: 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_connect

Without 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+.


from ably.realtime.connection import ConnectionEvent, ConnectionState
from ably.transport.defaults import Defaults
Expand All @@ -10,6 +18,68 @@
from test.ably.utils import BaseAsyncTestCase


async def _relay(src, dst):
try:
async for msg in src:
await dst.send(msg)
except Exception:
pass


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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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 = None

Note: 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.

self._close_event: asyncio.Event | None = None

async def _handler(self, client_ws):
# Create a fresh event for this connection; signal to drop the connection cleanly
self._close_event = asyncio.Event()
path = client_ws.request.path # e.g. "/?key=...&format=json"
target_url = f"wss://{self.target_host}{path}"
Comment on lines +41 to +42
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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.headers

ServerConnection.request is available after the opening handshake and contains the HTTP handshake Request object. In migration terms: connection.pathconnection.request.path (and request_headersrequest.headers). [1], [2], [3]


🏁 Script executed:

# Find websockets version requirement
git ls-files | grep -E "(requirements|setup|pyproject|poetry)" | head -20

Repository: 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.py

Repository: 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 -10

Repository: 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.py

Repository: 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.py

Repository: 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 -30

Repository: 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.py

Repository: 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.path exists (legacy server protocol object) — you can read it as websocket.path. The docs explicitly recommend using the .path attribute of the first handler argument instead of relying on a separate path parameter. [1]
  • The old handler signature async def handler(websocket, path): ... had its 2nd path parameter made optional earlier, and in websockets 13.0 it’s deprecated (you should switch to websocket.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 than WebSocketServerProtocol.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:


🏁 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 -20

Repository: 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.

try:
async with _ws_connect(target_url, ping_interval=None) as server_ws:
c2s = asyncio.create_task(_relay(client_ws, server_ws))
s2c = asyncio.create_task(_relay(server_ws, client_ws))
close_task = asyncio.create_task(self._close_event.wait())
try:
await asyncio.wait([c2s, s2c, close_task], return_when=asyncio.FIRST_COMPLETED)
finally:
c2s.cancel()
s2c.cancel()
close_task.cancel()
except Exception:
pass
Comment on lines +54 to +55
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

# After _handler returns the websockets server sends a normal close frame (1000)

async def close_active_connection(self):
"""Trigger a normal WS close (code 1000) on the currently active client connection.

Signals the handler to exit; the websockets server framework then sends the
close frame automatically when the handler coroutine returns.
"""
if self._close_event:
self._close_event.set()

@property
def endpoint(self) -> str:
"""Endpoint string to pass to AblyRealtime (combine with tls=False)."""
return f"127.0.0.1:{self.port}"

async def __aenter__(self):
self.server = await ws_serve(self._handler, "127.0.0.1", 0, ping_interval=None)
self.port = self.server.sockets[0].getsockname()[1]
return self

async def __aexit__(self, *args):
if self.server:
self.server.close()
await self.server.wait_closed()


class TestRealtimeConnection(BaseAsyncTestCase):
async def asyncSetUp(self):
self.test_vars = await TestApp.get_test_vars()
Expand Down Expand Up @@ -399,3 +469,37 @@ async def on_protocol_message(msg):
await asyncio.wait_for(ably.connection.once_async(ConnectionState.CONNECTED), timeout=5)

await ably.close()

async def test_normal_ws_close_triggers_immediate_reconnection(self):
"""Server normal WS close (code 1000) must trigger immediate reconnection.

Regression test: ConnectionClosedOK was silently swallowed and deactivate_transport
was never called, leaving the client disconnected until the idle timer fired.
"""
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=proxy.endpoint,
)
Comment on lines +479 to +485
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.


try:
await asyncio.wait_for(
ably.connection.once_async(ConnectionState.CONNECTED), timeout=10
)

# Simulate server sending a normal WS close frame
await proxy.close_active_connection()

# Must go CONNECTING quickly — not after the 25 s idle timer
await asyncio.wait_for(
ably.connection.once_async(ConnectionState.CONNECTING), timeout=1
)

# Must reconnect immediately — not after the 500 s retry timer
await asyncio.wait_for(
ably.connection.once_async(ConnectionState.CONNECTED), timeout=10
)
finally:
await ably.close()
2 changes: 1 addition & 1 deletion uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading