Skip to content

fix: replace panicking unwrap() calls with proper error handling in WebSocket code#1352

Open
sansyrox wants to merge 1 commit intomainfrom
fix/websocket-error-handling
Open

fix: replace panicking unwrap() calls with proper error handling in WebSocket code#1352
sansyrox wants to merge 1 commit intomainfrom
fix/websocket-error-handling

Conversation

@sansyrox
Copy link
Copy Markdown
Member

@sansyrox sansyrox commented Mar 27, 2026

Summary

  • WebSocketConnector::started/stopped used .unwrap() on router.get("connect"/"close") — panics if handler map is misconfigured. Now logs an error and continues.
  • execute_ws_function had a chain of 5+ .unwrap() calls that could panic on handler dispatch, future resolution, or result extraction. All replaced with match + log::error.
  • sync_send_to/async_send_to used Uuid::parse_str().unwrap() on user-provided IDs. Now returns early with an error log (sync) or raises PyValueError (async).
  • All println!() calls in WebSocket send/broadcast methods replaced with log::debug/log::error.

Test plan

  • All existing WebSocket integration tests pass (test_web_sockets.py)
  • Manual test: abruptly close a WebSocket client without a close frame — server should not crash
  • Manual test: rapidly connect/disconnect 20 times — server should remain stable

Made with Cursor

Summary by CodeRabbit

  • Bug Fixes
    • Improved WebSocket error handling to prevent crashes from invalid inputs and missing handlers.
    • Enhanced logging for WebSocket operations and message handling for better diagnostics and troubleshooting.

…ebSocket code

- WebSocketConnector::started/stopped used .unwrap() on router.get("connect"/"close")
  which would panic if the handler map was misconfigured. Now logs an error instead.
- execute_ws_function had a chain of 5+ unwrap() calls that could panic on handler
  dispatch, future resolution, or result extraction. All replaced with match + log::error.
- sync_send_to/async_send_to used Uuid::parse_str().unwrap() on user-provided IDs.
  Now returns early with an error log (sync) or PyValueError (async).
- All println!() calls in WebSocket send/broadcast methods replaced with log::debug/error.

Made-with: Cursor
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 27, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
robyn Ready Ready Preview, Comment Mar 27, 2026 0:07am

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

Two files updated to replace panic-prone unwrap() calls with proper error handling using match statements and conditional logic. Error paths now log errors and return early instead of panicking. Logging changed from println! to structured log::debug! and log::error! calls.

Changes

Cohort / File(s) Summary
WebSocket Executor Error Handling
src/executors/web_socket_executors.rs
Replaced unwrap() calls with match statements in async and sync handler paths; errors in Python handler binding, calling, and result extraction now logged with early returns instead of panics.
WebSocket Module Router & API Error Handling
src/websockets/mod.rs
Replaced unwrap() with conditional error handling for router handler retrieval during lifecycle events and recipient ID parsing; message send/broadcast APIs now propagate PyValueError or log errors instead of panicking; replaced println! with structured logging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰✨
Unwraps begone, let errors sing,
Match arms catch each panic's sting,
With logs to guide the fearless way,
Our WebSockets live another day! 🚀

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: replacing panicking unwrap() calls with proper error handling in WebSocket code.
Description check ✅ Passed The description provides a comprehensive summary of changes across multiple files, details the specific unwrap() replacements, includes a test plan, and follows the repository template structure with all key sections addressed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/websocket-error-handling

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/websockets/mod.rs (1)

78-88: Remaining unwrap() on Py::new() at line 86.

While out of scope for the changed lines, there's still a panicking unwrap() on Py::new() that could fail if Python object allocation fails. For consistency with the PR's goal of removing panic-prone code, consider handling this case:

♻️ Suggested improvement
         self.message_channel = Python::with_gil(|py| {
-            Some(
-                Py::new(
-                    py,
-                    WebSocketChannel {
-                        receiver: Arc::new(tokio::sync::Mutex::new(rx)),
-                    },
-                )
-                .unwrap(),
-            )
+            match Py::new(
+                py,
+                WebSocketChannel {
+                    receiver: Arc::new(tokio::sync::Mutex::new(rx)),
+                },
+            ) {
+                Ok(channel) => Some(channel),
+                Err(e) => {
+                    log::error!("Failed to create WebSocketChannel: {}", e);
+                    None
+                }
+            }
         });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/websockets/mod.rs` around lines 78 - 88, The assignment to
self.message_channel uses Py::new(...) inside Python::with_gil(...) and
currently calls unwrap() on Py::new, which can panic; change this to handle the
Result from Py::new instead of unwrapping: call Py::new(...).map(|pyobj|
Some(pyobj)).or_else(|err| { /* log or convert err and return None or propagate
*/ }) inside the Python::with_gil closure, or propagate an error from the
surrounding function; ensure you reference Py::new, WebSocketChannel,
Python::with_gil and self.message_channel when implementing the non-panicking
error handling (log or return a Result) so allocation failures don’t cause a
panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/websockets/mod.rs`:
- Around line 78-88: The assignment to self.message_channel uses Py::new(...)
inside Python::with_gil(...) and currently calls unwrap() on Py::new, which can
panic; change this to handle the Result from Py::new instead of unwrapping: call
Py::new(...).map(|pyobj| Some(pyobj)).or_else(|err| { /* log or convert err and
return None or propagate */ }) inside the Python::with_gil closure, or propagate
an error from the surrounding function; ensure you reference Py::new,
WebSocketChannel, Python::with_gil and self.message_channel when implementing
the non-panicking error handling (log or return a Result) so allocation failures
don’t cause a panic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 487d22cc-d81e-4443-a6b9-e08ad54e291e

📥 Commits

Reviewing files that changed from the base of the PR and between a54ff96 and 2a7b18f.

📒 Files selected for processing (2)
  • src/executors/web_socket_executors.rs
  • src/websockets/mod.rs

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 27, 2026

Merging this PR will not alter performance

✅ 189 untouched benchmarks


Comparing fix/websocket-error-handling (2a7b18f) with main (a54ff96)

Open in CodSpeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant