Fix: unify WebRTC connection tracking for API + UI connections #404
Conversation
|
@DeveloperViraj Thank you for taking a stab at the Issue I raised. However, the scope of the current MR should probably just include the logic in |
backend/fastrtc/stream.py
Outdated
| except Exception: | ||
| for k in wc.connections: | ||
| all_ids.add(str(k)) | ||
| for alt_name in ("_connections", "_conn_map", "active_connections"): |
There was a problem hiding this comment.
When are these naming conventions used? I cannot find them in the current code base.
backend/fastrtc/stream.py
Outdated
| for k in wc.connections.keys(): | ||
| all_ids.add(str(k)) | ||
| except Exception: | ||
| for k in wc.connections: |
There was a problem hiding this comment.
Does this ever happen?
|
Updated as per feedback Reverted to the original structure and retained all comments Kept the scope minimal, only added the get_all_connections() helper inside Stream Removed extra endpoints and unrelated edits Ready for re-review. Thanks for the detailed feedback, @marcusvaltonen 🙏 |
| self.server_rtc_configuration = self.convert_to_aiortc_format( | ||
| server_rtc_configuration | ||
| ) | ||
| self.verbose = verbose |
There was a problem hiding this comment.
Revert changes: self.verbose, self._ui shouldn't change
backend/fastrtc/stream.py
Outdated
| self.verbose = verbose | ||
| self._ui = self._generate_default_ui(ui_args) | ||
| self._ui.launch = self._wrap_gradio_launch(self._ui.launch) | ||
| def get_all_connections(self) -> List[str]: |
There was a problem hiding this comment.
This repo is using typing hints for Python 3.10+. It is enough (and preferred) to use [str]
backend/fastrtc/stream.py
Outdated
| self._ui.launch = self._wrap_gradio_launch(self._ui.launch) | ||
| def get_all_connections(self) -> List[str]: | ||
| all_ids = list(self.connections.keys()) # type: ignore | ||
| if self.webrtc_component and hasattr(self.webrtc_component, "connections"): |
There was a problem hiding this comment.
It is enough to check that if self.webrtc_component since it is guaranteed to have the attribute connections.
backend/fastrtc/stream.py
Outdated
| self._ui = self._generate_default_ui(ui_args) | ||
| self._ui.launch = self._wrap_gradio_launch(self._ui.launch) | ||
| def get_all_connections(self) -> List[str]: | ||
| all_ids = list(self.connections.keys()) # type: ignore |
backend/fastrtc/stream.py
Outdated
| def get_all_connections(self) -> List[str]: | ||
| all_ids = list(self.connections.keys()) # type: ignore | ||
| if self.webrtc_component and hasattr(self.webrtc_component, "connections"): | ||
| all_ids += list(self.webrtc_component.connections.keys()) # type: ignore |
backend/fastrtc/stream.py
Outdated
| self.verbose = verbose | ||
| self._ui = self._generate_default_ui(ui_args) | ||
| self._ui.launch = self._wrap_gradio_launch(self._ui.launch) | ||
| def get_all_connections(self) -> List[str]: |
There was a problem hiding this comment.
Missing docstring. See e.g., #405
|
These type of things should ideally have unit tests as well, but it is up to @freddyaboulton to decide that. |
…rs, and add docstring
|
Hey @marcusvaltonen ! Moved get_all_connections() out of init Restored self.verbose, _ui, and _ui.launch Switched to the modern list[str] type hint Cleaned up the extra # type: ignore Added a short docstring Should be all set now, let me know if anything else needs tweaking |
|
Hi @DeveloperViraj! Thank you for taking time to look at my Issue and implement a possible remedy. I think the MR looks good in terms of finding connections; however, please see my updated discussion in #403, explaining why it does not solve all further downstream tasks, and you still have to rely on some awkward hacking. I would recommend that we wait for @freddyaboulton to see this and share his thoughts, before you spend too much time going further. |
2 participants
Summary
Fixes issue #403 where WebRTC connections created via the Gradio UI were not visible through the Stream API endpoints.
What Changed
get_all_connections()inbackend/fastrtc/stream.pyto aggregate both server-side and UI-created connections./connectionsFastAPI route insideStream.mount()to expose all active WebRTC connections.Why
Ensures consistent connection tracking between UI and API, resolving sync issues in hybrid WebRTC setups.
Testing
GET /connections→ both UI + API connections appear.Closes #403