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

Unhandled WebSocketConnectionClosedException #383

Open
h4nsu opened this issue Mar 7, 2024 · 3 comments
Open

Unhandled WebSocketConnectionClosedException #383

h4nsu opened this issue Mar 7, 2024 · 3 comments

Comments

@h4nsu
Copy link
Contributor

h4nsu commented Mar 7, 2024

Occasionally this line:

self.__rpc_message_queue.append(json.loads(self.websocket.recv()))

blows up with WebSocketConnectionClosedException. Would it be ok to wrap it with an auto-reconnect try-catch just like here:
try:
self.websocket.send(json.dumps(payload))
except WebSocketConnectionClosedException:
if self.config.get('auto_reconnect') and self.url:
# Try to reconnect websocket and retry rpc_request
self.debug_message("Connection Closed; Trying to reconnecting...")
self.connect_websocket()
return self.rpc_request(method=method, params=params, result_handler=result_handler)
else:
# websocket connection is externally created, re-raise exception
raise

@arjanz
Copy link
Member

arjanz commented Mar 8, 2024

Yes handling this in the application is the preferred way. There is an 'auto_reconnect' feature built in (auto_reconnect=True during init), which will work as well, but will hide side-effects like subscriptions being reset from the application.

@h4nsu
Copy link
Contributor Author

h4nsu commented Mar 8, 2024

Sorry, what I wrote was confusing. Here's what I meant:

This:

self.websocket.send(json.dumps(payload))

is wrapped with a try-block and handles broken sockets transparently when auto_reconnect=True.

Shouldn't the same practice be applied here:

self.__rpc_message_queue.append(json.loads(self.websocket.recv()))

so that I don't need to reconnect manually in my application and I can simply rely on auto_reconnect doing what it promises? ;)

@arjanz
Copy link
Member

arjanz commented Mar 8, 2024

Ah I see what you mean now.. You are most likely right, now probably in case of a subscription with a long interval between responses the connection could be closed. The question is what will happen if during an active subscription the result_handler and its update_nr will be reset, I'll have to test if there are undesired side-effects.

If there aren't any I can just expand the try/catch and I'll link a PR, otherwise I think it's best to move to checks to the application, so there is more control what to do with these edge cases.

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

No branches or pull requests

2 participants