-
Notifications
You must be signed in to change notification settings - Fork 8
Websocket requests including supported entity types #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 28 commits
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
d6c5611
feat: Add requests for supported entity types, version and localization
kennymc-c df90062
Working handle of websocket requests from ucapi
albaintor b4de43b
Replaced exception with error trace
albaintor a139fed
Handle exception
albaintor fe8f47a
Fixed stacktrace error, all good now
albaintor c0b802b
Fixed blocking websocket requests : create task for each message to h…
albaintor e6e7cba
Removed response signatures
albaintor d0eae3c
Removed response signatures
albaintor 4a9e175
Check after supported entity types
albaintor 110d147
Moved extraction of supported entity types in request for available e…
damienbto e80291d
Linting
damienbto 7a3e8a7
Requested changes
albaintor 5e1ab8c
Linting and removed comment
albaintor 4a413fc
Linting flake8
albaintor 0caacfb
Merge branch 'main' into websocket_requests
zehnm 335b9a3
fix: merge from main
zehnm 55b4c45
Added media browsing request
albaintor 5140403
Added missing methods and commands
albaintor 6d0a3a5
Fixed search media response type
albaintor c1c4d9b
Merged new methods
albaintor 489a789
Merge branch 'websocket_requests' of https://github.com/albaintor/int…
albaintor 22b01ee
Added clients extraction (hack). To be improved
albaintor a4bbcf6
Create task is necessary to avoid blocking
albaintor e55a4b4
Refactoring and added typed definitions
albaintor 69ab522
Finalized & tested updated API with browsing/search support
albaintor 0665b31
Removed forced media_type to MediaContentType as it can be custom
albaintor 8f9f553
Fixed bug
albaintor 7c0e585
Fixed bug
albaintor 0081b3d
Nailed bug finally
albaintor 4c16fbf
Fixes reported by Jack
albaintor d11cfea
Fixes following Markus review
albaintor de39fa7
Merge branch 'media_browsing' into websocket_requests
albaintor 7f015ba
Merge branch 'main' into websocket_requests
zehnm 2f02593
merge cleanup
zehnm ab21f22
clean up comments
zehnm f83a0f1
no task required for _process_ws_binary_message
zehnm b9d3012
clean up: remove _supported_entity_types
zehnm 4ad5e15
Merge remote-tracking branch 'upstream/main' into websocket_requests
albaintor 9c41ae7
Changed code with queues management
albaintor 58fd854
Merge branch 'websocket_requests' of https://github.com/albaintor/int…
albaintor c47b9ef
Removed unecessary files
albaintor 3fb765e
Reverted unwanted changes
albaintor 8ca4a4a
..
albaintor 48d6aee
Handle of requests/response inside requests
albaintor d62dbe7
Linting
albaintor 4914534
Linting
albaintor 0907267
Linting
albaintor 649b042
Added requested changes
albaintor afb416b
Linting
albaintor ac2bc4c
refactor: log all exceptions when terminating WS tasks
zehnm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,13 +107,18 @@ def __init__(self, loop: AbstractEventLoop | None = None): | |
| self._available_entities = Entities("available", self._loop) | ||
| self._configured_entities = Entities("configured", self._loop) | ||
|
|
||
| self._req_id = 1 # Request ID counter for outgoing requests | ||
|
|
||
| self._voice_handler: VoiceStreamHandler | None = None | ||
| self._voice_session_timeout: int = self.DEFAULT_VOICE_SESSION_TIMEOUT_S | ||
| # Active voice sessions | ||
| self._voice_sessions: dict[VoiceSessionKey, _VoiceSessionContext] = {} | ||
| # Enforce: at most one active session per entity_id (across all websockets) | ||
| self._voice_session_by_entity: dict[str, VoiceSessionKey] = {} | ||
|
|
||
| # One receiver per websocket (already in _handle_ws). Responses are dispatched to futures here. | ||
| self._ws_pending: dict[Any, dict[int, asyncio.Future]] = {} | ||
|
|
||
| # Setup event loop | ||
| asyncio.set_event_loop(self._loop) | ||
|
|
||
|
|
@@ -216,6 +221,8 @@ async def _start_web_socket_server(self, host: str, port: int) -> None: | |
| async def _handle_ws(self, websocket) -> None: | ||
| try: | ||
| self._clients.add(websocket) | ||
| # Init per-websocket pending requests map | ||
| self._ws_pending[websocket] = {} | ||
| _LOG.info("WS: Client added: %s", websocket.remote_address) | ||
|
|
||
| # authenticate on connection | ||
|
|
@@ -227,9 +234,9 @@ async def _handle_ws(self, websocket) -> None: | |
| # Distinguish between text (str) and binary (bytes-like) messages | ||
| if isinstance(message, str): | ||
| # JSON text message | ||
| await self._process_ws_message(websocket, message) | ||
| asyncio.create_task(self._process_ws_message(websocket, message)) | ||
| elif isinstance(message, (bytes, bytearray, memoryview)): | ||
| # Binary message (protobuf in future) | ||
| # Binary message (protobuf) | ||
| await self._process_ws_binary_message(websocket, bytes(message)) | ||
| else: | ||
| _LOG.warning( | ||
|
|
@@ -270,7 +277,11 @@ async def _handle_ws(self, websocket) -> None: | |
| key[1], | ||
| ex, | ||
| ) | ||
|
|
||
| # Cancel all pending requests for this websocket (client disconnected) | ||
| pending = self._ws_pending.pop(websocket, {}) | ||
| for _, fut in pending.items(): | ||
| if not fut.done(): | ||
| fut.set_exception(ConnectionError("WebSocket disconnected")) | ||
| self._clients.remove(websocket) | ||
| _LOG.info("[%s] WS: Client removed", websocket.remote_address) | ||
| self._events.emit(uc.Events.CLIENT_DISCONNECTED, websocket=websocket) | ||
|
|
@@ -421,6 +432,99 @@ async def _process_ws_message(self, websocket, message) -> None: | |
| await self._handle_ws_request_msg(websocket, msg, req_id, msg_data) | ||
| elif kind == "event": | ||
| await self._handle_ws_event_msg(websocket, msg, msg_data) | ||
| elif kind == "resp": | ||
| # Response to a previously sent request | ||
| # Some implementations use "req_id", others use "id" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where did you see an |
||
| resp_id = data.get("req_id", data.get("id")) | ||
| if resp_id is None: | ||
| _LOG.warning( | ||
| "[%s] WS: Received resp without req_id/id: %s", | ||
| websocket.remote_address, | ||
| message, | ||
| ) | ||
| return | ||
|
|
||
| pending = self._ws_pending.get(websocket) | ||
| if not pending: | ||
| _LOG.debug( | ||
| "[%s] WS: No pending map for resp_id=%s (late resp?)", | ||
| websocket.remote_address, | ||
| resp_id, | ||
| ) | ||
| return | ||
| fut = pending.get(int(resp_id)) | ||
| if fut is None: | ||
| _LOG.debug( | ||
| "[%s] WS: Unmatched resp_id=%s (not pending). msg=%s", | ||
| websocket.remote_address, | ||
| resp_id, | ||
| msg, | ||
| ) | ||
| return | ||
|
|
||
| if not fut.done(): | ||
| fut.set_result(data) | ||
|
|
||
| async def _ws_request( | ||
| self, | ||
| websocket, | ||
| msg: str, | ||
| msg_data: dict[str, Any] | None = None, | ||
| *, | ||
| timeout: float = 10.0, | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Send a request over websocket and await the matching response. | ||
|
|
||
| - Uses a Future stored in self._ws_pending[websocket][req_id] | ||
| - Reader task (_handle_ws -> _process_ws_message) completes the future on 'resp' | ||
| - Raises TimeoutError on timeout | ||
| :param websocket: client connection | ||
| :param msg: event message name | ||
| :param msg_data: message data payload | ||
| :param timeout: timeout for message | ||
| """ | ||
| # Ensure per-socket structures exist (in case you call before _handle_ws init) | ||
| if websocket not in self._ws_pending: | ||
| self._ws_pending[websocket] = {} | ||
|
|
||
| req_id = self._req_id | ||
| self._req_id += 1 | ||
|
|
||
| fut = self._loop.create_future() | ||
| self._ws_pending[websocket][req_id] = fut | ||
|
|
||
| try: | ||
| payload: dict[str, Any] = {"kind": "req", "id": req_id, "msg": msg} | ||
| if msg_data is not None: | ||
| payload["msg_data"] = msg_data | ||
|
|
||
| if _LOG.isEnabledFor(logging.DEBUG): | ||
| _LOG.debug( | ||
| "[%s] ->: %s", | ||
| websocket.remote_address, | ||
| filter_log_msg_data(payload), | ||
| ) | ||
| await websocket.send(json.dumps(payload)) | ||
|
|
||
| # Await response | ||
| resp = await asyncio.wait_for(fut, timeout=timeout) | ||
| return resp | ||
|
|
||
| except asyncio.TimeoutError as ex: | ||
| _LOG.error( | ||
| "[%s] Timeout waiting for response to %s (req_id=%s) %s", | ||
| websocket.remote_address, | ||
| msg, | ||
| req_id, | ||
| ex, | ||
| ) | ||
| raise ex | ||
| finally: | ||
| # Cleanup pending future entry | ||
| pending = self._ws_pending.get(websocket) | ||
| if pending: | ||
| pending.pop(req_id, None) | ||
|
|
||
| async def _process_ws_binary_message(self, websocket, data: bytes) -> None: | ||
| """Process a binary WebSocket message using protobuf IntegrationMessage. | ||
|
|
@@ -687,6 +791,7 @@ async def _voice_session_timeout_task(self, key: VoiceSessionKey) -> None: | |
| async def _handle_ws_request_msg( | ||
| self, websocket, msg: str, req_id: int, msg_data: dict[str, Any] | None | ||
| ) -> None: | ||
| # pylint: disable=R0912 | ||
| if msg == uc.WsMessages.GET_DRIVER_VERSION: | ||
| await self._send_ws_response( | ||
| websocket, | ||
|
|
@@ -929,7 +1034,9 @@ async def _entity_command( | |
| entity.id, | ||
| ) | ||
| result = await entity.command( | ||
| cmd_id, msg_data["params"] if "params" in msg_data else None | ||
| cmd_id, | ||
| msg_data["params"] if "params" in msg_data else None, | ||
| websocket=websocket, | ||
| ) | ||
|
|
||
| await self.acknowledge_command(websocket, req_id, result) | ||
|
|
@@ -1351,10 +1458,67 @@ def remove_all_listeners(self, event: uc.Events | None) -> None: | |
| """ | ||
| self._events.remove_all_listeners(event) | ||
|
|
||
| async def get_supported_entity_types( | ||
| self, websocket, *, timeout: float = 5.0 | ||
| ) -> list[str]: | ||
|
zehnm marked this conversation as resolved.
|
||
| """Request supported entity types from client and return msg_data.""" | ||
| resp = await self._ws_request( | ||
| websocket, | ||
| "get_supported_entity_types", | ||
| timeout=timeout, | ||
| ) | ||
| if resp.get("msg") != "supported_entity_types": | ||
| _LOG.debug( | ||
| "[%s] Unexpected resp msg for get_supported_entity_types: %s", | ||
| websocket.remote_address, | ||
| resp.get("msg"), | ||
| ) | ||
| return resp.get("msg_data", []) | ||
|
|
||
| async def get_version(self, websocket, *, timeout: float = 5.0) -> dict[str, Any]: | ||
| """Request client version and return msg_data.""" | ||
| resp = await self._ws_request( | ||
| websocket, | ||
| "get_version", | ||
| timeout=timeout, | ||
| ) | ||
| if resp.get("msg") != "version": | ||
| _LOG.debug( | ||
| "[%s] Unexpected resp msg for get_version: %s", | ||
| websocket.remote_address, | ||
| resp.get("msg"), | ||
| ) | ||
|
|
||
| return resp.get("msg_data") | ||
|
|
||
| async def get_localization_cfg( | ||
| self, websocket, *, timeout: float = 5.0 | ||
| ) -> dict[str, Any]: | ||
| """Request localization config and return msg_data.""" | ||
| resp = await self._ws_request( | ||
| websocket, | ||
| "get_localization_cfg", | ||
| timeout=timeout, | ||
| ) | ||
|
|
||
| if resp.get("msg") != "localization_cfg": | ||
| _LOG.debug( | ||
| "[%s] Unexpected resp msg for get_localization_cfg: %s", | ||
| websocket.remote_address, | ||
| resp.get("msg"), | ||
| ) | ||
|
|
||
| return resp.get("msg_data") | ||
|
|
||
| ############## | ||
| # Properties # | ||
| ############## | ||
|
|
||
| @property | ||
| def clients(self) -> set: | ||
| """Return all clients.""" | ||
| return self._clients.copy() | ||
|
|
||
| @property | ||
| def client_count(self) -> int: | ||
| """Return number of WebSocket clients.""" | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After further consideration, creating a dedicated task per received JSON message is not a good solution and can create more issues than it solves.
I suggest a proper solution with a consumer/producer model with queues. One task reads from the socket and enqueues messages, one task writes outbound messages, and worker tasks or inline logic process messages from the queue. This keeps websocket access centralized, gives you explicit backpressure, and makes ordering and shutdown behavior much easier to control.
A simple demo implementation based on the Websockets library documentation https://websockets.readthedocs.io/en/15.0.1/howto/patterns.html