-
Notifications
You must be signed in to change notification settings - Fork 122
improve async websocket examples #897
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes narrow exception handling in an async websocket client by replacing broad except clauses with targeted handlers. KeyboardInterrupt is explicitly re-raised for cancellation, while other exceptions trigger reconnection logic. A try/finally block ensures listener task cleanup regardless of success or failure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
230256e to
6fa321e
Compare
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
xrpl/asyncio/clients/async_websocket_client.py (1)
196-207: Good addition of cleanup logic with try/finally block.The
try/finallyblock ensures the listener task is cancelled regardless of how the function exits. This is essential for proper resource management in async code.Consider awaiting the cancelled task to avoid "Task was destroyed but it is pending" warnings and ensure clean cancellation:
🔎 Proposed enhancement for cleaner task cancellation
finally: # whether or not there is an error, cancel the listener listener.cancel() + try: + await listener + except asyncio.CancelledError: + pass
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
xrpl/asyncio/clients/async_websocket_client.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-15T00:41:02.631Z
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 773
File: tests/integration/reusable_values.py:112-119
Timestamp: 2025-01-15T00:41:02.631Z
Learning: In xrpl-py integration tests, errors during test setup should be allowed to terminate the test suite naturally to aid debugging, rather than being caught and wrapped in custom error handlers. This applies to functions like `sign_and_reliable_submission_async` which already have robust error handling.
Applied to files:
xrpl/asyncio/clients/async_websocket_client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Integration test (3.10)
- GitHub Check: Integration test (3.9)
- GitHub Check: Integration test (3.11)
- GitHub Check: Integration test (3.13)
- GitHub Check: Integration test (3.14)
- GitHub Check: Integration test (3.12)
- GitHub Check: Integration test (3.8)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
xrpl/asyncio/clients/async_websocket_client.py (1)
139-145: Excellent improvement to exception handling specificity.Replacing the bare
except:withexcept Exception:is the correct approach. This allowsKeyboardInterruptand otherBaseExceptionsubclasses to propagate properly, enabling users to cancel the async context with Ctrl-C.
| except KeyboardInterrupt: | ||
| # allow crtl-c to actually cancel | ||
| raise | ||
| except Exception: | ||
| # anything else reconnects | ||
| print("Lost connection! Reconnecting") |
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.
Excellent pattern for supervisor loop with proper cancellation support.
The explicit KeyboardInterrupt handling enables users to cleanly exit the supervisor loop with Ctrl-C, while all other exceptions trigger reconnection. This is the recommended pattern for websocket supervisors.
Minor typo: "crtl-c" → "ctrl-c" on line 179.
🤖 Prompt for AI Agents
In xrpl/asyncio/clients/async_websocket_client.py around lines 178 to 183, there
is a minor typo in the inline comment text: "crtl-c" should be "ctrl-c"; update
the comment to read "allow ctrl-c to actually cancel" (preserve
capitalization/style) so the docstring/comment is correct while leaving the
exception handling logic unchanged.
High Level Overview of Change
quick improvement to examples of async websocket code
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan