Skip to content

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


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

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