Suppress ClientConnectionResetError when returning logs #5358
Conversation
When client requests (or more often follows) some busy logs and closes the connection while the StreamWriter tries to write into it, an exception is raised. This should be harmless, and unless there's another way to handle this gracefully (I'm not aware of), then it should be safe to ignore the exception in this context.
📝 WalkthroughWalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Logger
Client->>API: Request logs
API->>Logger: Stream logs
alt Client connection reset
Logger->>API: ClientConnectionResetError
API->>API: Suppress ClientConnectionResetError
else Fetching data
API->>Logger: Fetch data from systemd-journald
alt Connection reset occurs
Logger->>API: ConnectionResetError
API->>API: Raise APIError
end
end
API->>Client: Send logs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
supervisor/api/host.py (1)
263-266: LGTM: Error handling improved as intended.The changes effectively suppress the
ClientConnectionResetErroras described in the PR objectives. The comment explains the rationale clearly, and thesuppresscontext manager is used correctly.However, to improve code readability and maintainability, consider extracting the error suppression logic into a separate helper method. This would allow for easier testing and potential reuse if needed elsewhere.
Here's a suggested refactor:
def _write_log_line_suppressing_client_reset(self, response, line): with suppress(ClientConnectionResetError): return await response.write(line.encode("utf-8") + b"\n") # Then in the method: await self._write_log_line_suppressing_client_reset(response, line)This refactoring would make the main method cleaner and the error handling logic more modular.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- supervisor/api/host.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
supervisor/api/host.py (2)
7-7: LGTM: Import statement added correctly.The import of
ClientConnectionResetErrorfromaiohttpis necessary for the new error handling in theadvanced_logs_handlermethod. It's placed appropriately with other imports from the same library.
Line range hint
1-266: Consider adding tests for the new error handling.While the changes look good and align with the PR objectives, I noticed that no new tests were added. Given that this change involves error handling, it would be beneficial to add unit tests to verify the behavior of the
advanced_logs_handlermethod with the new error suppression.Here's a script to check for test files related to this module:
This script will help identify if there are existing test files where you could add new tests for this functionality.
Proposed change
When client requests (or more often follows) some busy logs and closes the connection while the StreamWriter tries to write into it, an exception is raised. This should be harmless, and unless there's another way to handle this gracefully (I'm not aware of), then it should be safe to ignore the exception in this context.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes