Skip to content
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

Handle EOFError and ConnectionResetError without chaining exceptions in server connection failure #273

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

alonkukl
Copy link
Contributor

@alonkukl alonkukl commented Sep 4, 2024

This PR updates the exception handling logic in the client-server connection code. Specifically, it modifies how EOFError and ConnectionResetError are handled during connection failures. The goal is to avoid exception chaining (from e) for these specific error types, while maintaining chaining for all other exceptions.

Changes

  • Added conditional logic to raise RuntimeError without from e when the exception is EOFError or ConnectionResetError.

  • For all other exceptions, the existing behavior (raising RuntimeError with from e) is preserved.

Rationale

  • EOFError and ConnectionResetError indicate specific situations where the connection is lost in a more predictable way (e.g., server gracfully closing the connection).

  • Avoiding exception chaining for these cases provides a clearer stack trace, since the original exception doesn't add much value in these specific cases.

  • For other unexpected errors, preserving from e helps retain context for debugging.

Copy link
Member

@edyounis edyounis left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@edyounis edyounis merged commit 5176210 into main Sep 8, 2024
17 checks passed
@edyounis edyounis deleted the change_server_close_error branch September 8, 2024 13:22
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.

2 participants