Skip to content

Conversation

@fselmo
Copy link
Collaborator

@fselmo fselmo commented Jul 12, 2024

What was wrong?

Implement similar logic made in #3424 for v7 / main branch, gracefully handling the case of ConnectionClosedOK for websocket connections.

How was it fixed?

  • If a persistent connection is closed gracefully, log and raise a silent PersistentConnectionClosedOK exception, triggering an end to the message listener task and breaking out of the process_subscriptions() iterator.

Since we have very different socket logic between AsyncIPCProvider and WebSocketProvider, handle the specific websocket case inside the _provider_specific_message_listener() method. For WebSocket case, if a ConnectionClosedOK is raised, silence it and re-raise a PersistentConnectionClosedOK.

I don't currently see an equivalent case for AsyncIPCProvider so this pattern isn't utilized for that specific provider.

Todo:

Cute Animal Picture

20240714_125244

@fselmo fselmo force-pushed the port-3424-to-main branch 2 times, most recently from ef8a38e to 3c6e9dc Compare July 15, 2024 19:00
@fselmo fselmo marked this pull request as ready for review July 15, 2024 19:22
- Similar changes as ethereum#3424 that were implemented for ``v6``.
- Raise ``PersistentConnectionClosedOK`` if a ``ConnectionClosedOK`` is
  raised by the server for a websocket provider. Handle this appropriately
  in a pattern than can be applicable to other persistent connections
  by creating a new internal exception that is then handled, rather than
  handling the specific websocket exception.
@fselmo fselmo force-pushed the port-3424-to-main branch from 3c6e9dc to e798e76 Compare July 15, 2024 19:43
@fselmo fselmo requested review from kclowes, pacrob and reedsa July 15, 2024 19:55
@fselmo fselmo changed the title Port changes from #3424 to main / v7: Port similar changes from #3424 to main / v7: Jul 15, 2024
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

lgtm, 🐑 it!

@fselmo fselmo merged commit e9ecd9d into ethereum:main Jul 16, 2024
@fselmo fselmo deleted the port-3424-to-main branch July 16, 2024 16:37
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