Conversation
Summary of ChangesHello @mazz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request brings the application up-to-date with the latest Elixir and Erlang/OTP versions, along with a comprehensive upgrade of the Membrane framework and its associated plugins. These updates ensure compatibility with modern runtime environments and leverage the latest features and improvements from the underlying libraries. Additionally, a new Python test client has been integrated to facilitate more efficient testing and interaction with the application's real-time communication features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request brings the application up to date with Elixir 1.18 and recent versions of the Membrane framework, which is a significant and necessary update. The removal of the membrane_telemetry dependency and the introduction of a new stats mechanism via a GenServer call is a good simplification. The addition of a Python test client is also valuable for testing and demonstration.
My review focuses on a few areas for improvement:
- A potential resource leak in
room.exdue to a missing call toEngine.remove_peer. - Some leftover commented-out code in
application.exthat should be removed for cleanliness. - A minor issue in the new Python test client that could cause it to hang.
- A potential typo in the environment variables for development.
Overall, this is a great step forward in modernizing the application. Addressing these points will help ensure the application is robust and maintainable.
| |> Enum.find(fn {_peer_id, peer_channel_pid} -> peer_channel_pid == pid end) | ||
|
|
||
| Engine.remove_peer(state.rtc_engine, peer_id) | ||
| {_elem, state} = pop_in(state, [:peer_channels, peer_id]) |
There was a problem hiding this comment.
The call to Engine.remove_peer/2 has been removed from this handle_info clause for a :DOWN message. When a peer's channel process goes down, the peer is no longer being explicitly removed from the RTC Engine. This could lead to a resource leak within the engine, as it might not clean up the peer's resources automatically. If this was not an intentional change due to an update in the membrane_rtc_engine API, Engine.remove_peer(state.rtc_engine, peer_id) should be restored to prevent potential leaks.
Engine.remove_peer(state.rtc_engine, peer_id)
{_elem, state} = pop_in(state, [:peer_channels, peer_id])
backend/lib/free4chat/application.ex
Outdated
| # defp config_common_dtls_key_cert() do | ||
| # {:ok, pid} = ExDTLS.start_link(client_mode: false, dtls_srtp: true) | ||
| # {:ok, pkey} = ExDTLS.get_pkey(pid) | ||
| # {:ok, cert} = ExDTLS.get_cert(pid) | ||
| # :ok = ExDTLS.stop(pid) | ||
| # Application.put_env(:free4chat, :dtls_pkey, pkey) | ||
| # Application.put_env(:free4chat, :dtls_cert, cert) | ||
| # end | ||
|
|
||
| # defp config_common_dtls_key_cert() do | ||
| # opts = [client_mode: false, dtls_srtp: true] | ||
|
|
||
| # # Modern ExDTLS does not use processes, instead creates a context struct | ||
| # {:ok, ctx} = ExDTLS.Context.init(opts) | ||
| # {:ok, pkey} = ExDTLS.Context.get_pkey(ctx) | ||
| # {:ok, cert} = ExDTLS.Context.get_cert(ctx) | ||
| # # No need to stop context for stateless API | ||
|
|
||
| # Application.put_env(:free4chat, :dtls_pkey, pkey) | ||
| # Application.put_env(:free4chat, :dtls_cert, cert) | ||
| # end |
There was a problem hiding this comment.
These large blocks of commented-out code should be removed to improve code clarity and maintainability. If this code is being kept for historical reference, it can be found in the version control history.
backend/source_dev_env
Outdated
| export DASHBOARD_AUTH_USERNAME=-admin; | ||
| export DASHBOARD_AUTH_PASSWORD=-admin; |
There was a problem hiding this comment.
The values for DASHBOARD_AUTH_USERNAME and DASHBOARD_AUTH_PASSWORD start with a hyphen (-). This is unusual and looks like a typo. If it's intentional, a comment explaining the reason would be helpful. Otherwise, the hyphens should be removed to avoid potential issues with basic authentication parsing.
export DASHBOARD_AUTH_USERNAME=admin;
export DASHBOARD_AUTH_PASSWORD=admin;
| async def receive_ws_events(): | ||
| try: | ||
| while True: | ||
| msg = await ws.recv() | ||
| logger.info(f"Received: {msg}") | ||
| except (asyncio.CancelledError, websockets.ConnectionClosed): | ||
| pass | ||
|
|
||
| # Run both user input and websocket receives concurrently | ||
| await asyncio.gather(read_user_input(), receive_ws_events()) |
There was a problem hiding this comment.
The join_room function uses asyncio.gather to run read_user_input and receive_ws_events concurrently. When the user types /quit, read_user_input finishes, but receive_ws_events is in an infinite loop and will continue to run. This causes gather to hang, and the program will not exit cleanly. The tasks should be managed so that when one exits, the other is cancelled.
Consider using asyncio.wait with return_when=asyncio.FIRST_COMPLETED to handle this gracefully. Additionally, the receive_ws_events function can be simplified by using an async for loop over the websocket connection.
async def receive_ws_events():
try:
async for msg in ws:
logger.info(f"Received: {msg}")
except (asyncio.CancelledError, websockets.ConnectionClosed):
pass
# Run both user input and websocket receives concurrently
input_task = asyncio.create_task(read_user_input())
receive_task = asyncio.create_task(receive_ws_events())
done, pending = await asyncio.wait(
[input_task, receive_task], return_when=asyncio.FIRST_COMPLETED
)
for task in pending:
task.cancel()
1 participant
Get app working with elixir 1.18.0-otp-27 and recent membrane. Remove membrane telemetry dependency. Include a python test client.
NOTE: I had to remove TURN to get it all working.