Skip to content

[depthcache] Propagate exception to user instead of catching and silently failing #1578

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

Conversation

ahorn639
Copy link

This error occurs frequently whilst using the DepthCacheManager
2025-04-14 01:06:23.479 binance.ws.reconnecting_websocket ERROR [MainThread] [_read_loop] BinanceWebsocketClosed (Connection closed. Reconnecting...)

The code takes too long to reconnect and in that time the message queue fills up and you get an BinanceWebsocketQueueOverflow exception
2025-04-14 01:07:02.926 binance.ws.reconnecting_websocket ERROR [MainThread] [_read_loop] Unknown exception (Message queue size 100 exceeded maximum 100)

All exceptions are caught internally by the library so the user can't automatically reconnect (this should really be fixed in the ReconnectingWebsocket but I don't know enough about all the various use cases to comment.
Instead, what happens is this repeatedly
2025-04-14 01:08:07.641 binance.ws.depthcache WARNING [MainThread] [recv]
2025-04-14 01:08:07.641 binance.ws.depthcache WARNING [MainThread] [recv]
2025-04-14 01:08:07.641 binance.ws.depthcache WARNING [MainThread] [recv]
2025-04-14 01:08:07.641 binance.ws.depthcache WARNING [MainThread] [recv]
...

This change just propagates the exception while reading off the queue to the user so that they can reconnect themselves

@ahorn639 ahorn639 requested a review from carlosmiei as a code owner April 14, 2025 12:15
@carlosmiei
Copy link
Collaborator

@ahorn639 Thanks for your contribution, we will review it and merge it shortly.

@pcriadoperez
Copy link
Collaborator

yeah.. I've had a similar question for the reconnects, but I'm not sure this is the right approach
With the reconnecting Websocket on a disconnect we send a message back to the user and the WS tries to reconnect on it's own.
In this case I believe the issue is that the depthCache is capturing the error message, and closing the Webscoket. So it's the worst of both worlds. It does not notify the user and stops the automatic reconnect.

I think here the correct behavior should be:
In case of a disconnect:

  • Pass the error message through the websocket to the user
  • Reconnect automatically and reinitlize the orderbook as it will be out of sync
  • If it fails to reconnect after 5 attemps. Return an error message through the ws and close the loop.

I believe this way the depthManager would follow the same logic as a regular websocket connection of the library.

I'll work on a PR to fix this

@pcriadoperez
Copy link
Collaborator

pcriadoperez commented Apr 20, 2025

@ahorn639, I opened a PR here that I believe resolves this issue. I think we can close this PR?
#1579

@ahorn639
Copy link
Author

ahorn639 commented May 4, 2025

@ahorn639, I opened a PR here that I believe resolves this issue. I think we can close this PR? #1579

Thanks @pcriadoperez ! Will give it a go, fix looks good

@ahorn639 ahorn639 closed this May 4, 2025
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