Skip to content

try to reconect on some exceptions #1594

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cogisum
Copy link

@cogisum cogisum commented Jul 18, 2025

Give it a chance to reconnect on some non-fatal exceptions.

@cogisum cogisum requested a review from carlosmiei as a code owner July 18, 2025 12:52
@carlosmiei carlosmiei requested a review from Copilot July 22, 2025 11:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the WebSocket reconnection logic to handle additional exceptions as recoverable instead of fatal, allowing the connection to automatically retry rather than completely failing.

  • Adds ConnectionClosedOK exception handling for graceful connection closures
  • Moves BinanceWebsocketUnableToConnect from fatal to recoverable exception category

@@ -15,9 +15,9 @@
pass

try:
from websockets.exceptions import ConnectionClosedError # type: ignore
from websockets.exceptions import ConnectionClosedError, ConnectionClosedOK # type: ignore
Copy link
Preview

Copilot AI Jul 22, 2025

Choose a reason for hiding this comment

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

The PR title contains a spelling error: 'reconect' should be 'reconnect'.

Copilot uses AI. Check for mistakes.

@carlosmiei carlosmiei closed this Jul 23, 2025
@carlosmiei carlosmiei reopened this Jul 23, 2025
Copy link
Collaborator

@pcriadoperez pcriadoperez left a comment

Choose a reason for hiding this comment

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

Hi @cogisum.

Thanks for the PR!

I believe it's correct to break the loop when throwing BinanceWebsocketUnableToConnect as this means the loop has tried to reconnect several times unsuccesfully.

As for the ConnectionClosedOK exception, I believe it's also correct to break the loop as the connection was closed cleanly.

Could you explain a bit more the issue you are trying to solve?

@pcriadoperez pcriadoperez self-assigned this Jul 28, 2025
@cogisum cogisum closed this Jul 28, 2025
@cogisum cogisum reopened this Jul 28, 2025
@cogisum
Copy link
Author

cogisum commented Jul 28, 2025

Hi @pcriadoperez , the PR is for ConnectionClosedOK, which I have met many times. The closing action was not initiated by me, so I think recovery needed.

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.

3 participants