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

WebsocketProviderV2 recv() lock + other multi-tasking support #3125

Merged
merged 11 commits into from
Oct 18, 2023

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Oct 12, 2023

What was wrong?

Some use cases for WebsocketProviderV2 don't allow asyncio.gather(tasks...), for example, as it will halt when two coroutines try to call recv() on the websocket connection at once. This PR attempts to ameliorate these scenarios.

  • Add an asyncio.Lock around the websocket recv() method.

  • Change listen_to_websocket() so that it only listens to subscription (or one-to-many request / response) messages. This is because every make_request() expects a 1-to-1 response-to-request and expects it to be returned on the same line e.g. variable_assignment = await w3.eth.block_number. If we are persistently listening to the socket, we clearly expect many messages. So, we only listen to messages from one-to-many calls, triggered by a ws.send().

  • Add an API for retrieving active subscriptions: w3.ws.subscriptions (WebsocketConnection.subscriptions)

  • Because of the separation of 1-to-1 request / response and one-to-many send() to recv() messages, separate the RequestProcessor caches into two types of "caches". The subscription cache is really a deque that always uses popleft(), so make it one. The response cache is looking for a particular cache key so this still makes sense to keep as an OrderedDict.

  • Defines an __await__() on the _PersistentConnectionWeb3 class so the AsyncWeb3.persistent_websocket() instantiation can be awaited
    (i.e. w3 = await AsyncWeb3.persistent_websocket(WebsocketProviderV2(...))). New integration test suite added with this pattern of instantiation.

Todo:

Cute Animal Picture

snake socks

    Y
  .-^-.
 /     \      .- ~ ~ -.
()     ()    /   _ _   `.                     _ _ _
 \_   _/    /  /     \   \                . ~  _ _  ~ .
   | |     /  /       \   \             .' .~       ~-. `.
   | |    /  /         )   )           /  /             `.`.
   \ \_ _/  /         /   /           /  /                `'
    \_ _ _.'         /   /           (  (               |=====|
                    /   /             \  \`             |=====|
                   /   /               \  \             |     |
                  /   /                 )  )            |      \___
                 (   (                 /  /             (        (_)
                  `.  `.             .'  /               ``````````
                    `.   ~ - - - - ~   .'
                       ~ . _ _ _ _ . ~

@fselmo fselmo force-pushed the websocket-v2-recv-lock branch 9 times, most recently from 4507da5 to a0752bc Compare October 16, 2023 19:34
fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 16, 2023
@fselmo fselmo marked this pull request as ready for review October 16, 2023 19:55
fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 16, 2023
fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 16, 2023
fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 16, 2023
fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 16, 2023
- Bonus: Add API for retrieving active subscriptions
- If we are listening to a websocket persistently, we shouldn't worry about timeouts in the same sense as a request that is waiting for a response. Instead, keep looping through the cycle of checking the ubscriptions deque / cache and trying to recv() on the websocket connection until something comes up.
- define an ``__await__()`` method that allows for awaiting a persistent connection to the websocket; e.g. ``w3 = await AsyncWeb3.persistent_websocket(WebsocketProviderV2)``
fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 16, 2023
- Add the beginnings of the documentation for why and how ``PersistentConnectionProvider`` instances cache request information and responses in order to process them appropriately.
- Link to the ``RequestProcessor`` documentation from the ``WebsocketProviderV2`` docs.
- One-to-many response logic doesn't use the request_timeout for the ``recv()`` since it indefinitely listens to the open websocket and with each turn of the while loop it needs to check the cache again. Use a similar logic for one-to-one requests except add the request timeout around the entirety of the while loop. This makes it so that we look for a response throughout the defined request_timeout time but we make sure to constantly check both the cache and call ``recv()`` on the websocket, in case the response has snuck into the cache from another request made somewhere else or from a persistent listener set up to receive messages.
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

Nothing major, looks good overall!

docs/internals.rst Outdated Show resolved Hide resolved
@@ -524,6 +527,8 @@ def persistent_websocket(
class _PersistentConnectionWeb3(AsyncWeb3):
provider: PersistentConnectionProvider

# w3 = AsyncWeb3.persistent_websocket(provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments that follow kind of look like code that was commented out. Recommend creating comment blocks that call these examples out more explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really only meant these to be for organization / reference. The websockets library does a similar thing with their connect() method here. I feel like we could let the documentation do the explaining. Thoughts?

web3/manager.py Show resolved Hide resolved
docs/internals.rst Outdated Show resolved Hide resolved
docs/internals.rst Outdated Show resolved Hide resolved
docs/internals.rst Outdated Show resolved Hide resolved
docs/internals.rst Outdated Show resolved Hide resolved
docs/providers.rst Outdated Show resolved Hide resolved
docs/providers.rst Outdated Show resolved Hide resolved
@@ -524,6 +527,8 @@ def persistent_websocket(
class _PersistentConnectionWeb3(AsyncWeb3):
provider: PersistentConnectionProvider

# w3 = AsyncWeb3.persistent_websocket(provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like these examples of usage. Would a label, something like # Example usage: or # Access this method with pattern: make it more clear for users?

Copy link
Collaborator Author

@fselmo fselmo Oct 17, 2023

Choose a reason for hiding this comment

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

I'd like to leave that up to the documentation. This is more so we separate the logic for each of those design patterns into code blocks and so contributors know where to look for it.

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.

Nothing came up in messing with it. Once existing notes are handled, lgtm!

@fselmo fselmo merged commit 9c88416 into ethereum:main Oct 18, 2023
84 checks passed
@fselmo fselmo deleted the websocket-v2-recv-lock branch October 18, 2023 16:44
fselmo added a commit that referenced this pull request Oct 18, 2023
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.

None yet

3 participants