WebsocketProviderV2 updates + formatting middleware bugfix #3116

fselmo · 2023-10-04T23:53:00Z

What was wrong?

Closes WebsocketProviderV2 field 'extraData' #3093
WebsocketProviderV2 needed tests for subscriptions. While we revamp the test suite and try to get some actual integration tests for eth_subscribe, introduce some static tests to make sure our formatters and middleware work appropriately for subscriptions.
Adds a timeout around trying to match the response id to the request id for a request. Also improves on the logic around matching the ids, simplifying it.
Add unit tests for the raw response cache process.
- Ensure we are caching any undesired responses before returning the desired response.
- Ensure we return the desired cached response, if cached, and that we never call recv() in that case.
- Test the timeout if a response with matching id never comes (see above bullet point for test case).
(breaking change for beta provider) Refactor to a proper AsyncIterator pattern for receiving a persistent message stream from the socket.
Update docs based on the above

Todo:

Add entry to the release notes

Cute Animal Picture

- Add a proper ``__anext()__`` setup for listening to an open websocket connection. This is the more appropriate pattern without the necessity for a while loop. This also more directly mirrors the ``websockets`` library example and design pattern. - Update documentation example to reflect the above changes.

- Change async_generator -> async_ctx_manager (Context Manager) as this is a more appropriate name for the pattern. - Create a PersistentConnectionProviderTest class and have both the context manager and iterator test suite run these tests. - Add static eth_subscribe tests to keep our formatters and middlewares in check. + additional improvements to typing; pass linting

- Simplify the json encoding for the static eth_subscription tests - Connect to websocket when entering the AsyncIterator pattern for WebsocketsProviderV2 if it isn't already connected

- We create transient caches on a per-provider basis. This wouldn't be shared among threads to begin with. The async_lock() utility was designed because the request session cache exists outside of any class and thus is shared across many instances of classes that utilize it. Change the async lock around transient caches to be a standard use of the ``asyncio.Lock()``. - Move the lock into the RequestProcessor class since that's where the caches live.

reedsa

Remaining nits, looking good otherwise!

docs/providers.rst

web3/_utils/module_testing/persistent_connection_provider.py

- We weren't caching every time we held a response. This lead to missing some messages entirely from the provider. This will need some better testing to ensure this logic never breaks. - When popping a raw response, actually return it. I'll add a test for this in a separate commit as well.

- Test that we cache any undesired messages until we find the one we want and return it. - Test that we return the cached response if in cache without ever calling recv()

- Add tests for ``syncing`` eth_subscribe requests - Tweak formatters keyset match to 75% match. Across all eth_subscribe requests, the keys are not close to being similar so we can afford a more relaxed check here.

web3/providers/websocket/websocket_v2.py

- Use the call timeout for the provider to set up a reasonable escape time for the while look that tries to match the response `id` with the request `id`. All providers should technically return matching ids in the response but we've run into issues with this in the past and this prevents an infinite loop, handling it somewhat gracefully. - Make sure that the ``WebsocketProviderV2`` sets a default timeout.

pacrob

lgtm!

reedsa · 2023-10-06T20:42:42Z

web3/providers/websocket/websocket_v2.py

+                if self.call_timeout and self.call_timeout <= 20
+                else 20


Should use the DEFAULT_PERSISTENT_CONNECTION_TIMEOUT constant here.

Suggested change

if self.call_timeout and self.call_timeout <= 20

else 20

if self.call_timeout and self.call_timeout <= DEFAULT_PERSISTENT_CONNECTION_TIMEOUT

else DEFAULT_PERSISTENT_CONNECTION_TIMEOUT

Isn't this an entirely different bit of logic though? What if we agree to change the default timeout? I think that was the point of hard-coding this here.

Let's say I don't want each call to the provider to take more than, say, 5 seconds. I wonder if I'm subscribed to a lot of subscriptions, if the response to my call won't be bogged down by the latency behind so many subscriptions where it may actually take longer than a call timeout to receive it.

Thoughts there?

That's fine then, I have no concerns with using the hardcoded value as is. Definitely would be overkill to create yet another constant just for this logic.

Yeah in the same way I think using self.call_timeout feels a bit disingenuous to use here too but at least the user can tweak that to make sense if desired. Definitely open to re-thinking this a bit more before ripping off the beta tag.

reedsa

🚀

fselmo added 3 commits October 4, 2023 11:24

Add async_geth_poa_middleware for subscription responses

0e5e996

fselmo changed the title ~~Properly apply formatting middleware to subscriptions~~ WebsocketProviderV2 updates + formatting middleware bugfix Oct 4, 2023

fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 4, 2023

relevant newsfragments for ethereum#3116

15bac16

fselmo added 2 commits October 4, 2023 18:05

Add a test for async_geth_poa_middleware w/ subscriptions

a07a6b3

relevant newsfragments for ethereum#3116

58e5576

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from 15bac16 to 58e5576 Compare October 5, 2023 00:05

fselmo marked this pull request as ready for review October 5, 2023 00:16

fselmo requested review from pacrob and reedsa October 5, 2023 00:16

fselmo added 2 commits October 5, 2023 09:58

Minor additions

ccbd868

- Simplify the json encoding for the static eth_subscription tests - Connect to websocket when entering the AsyncIterator pattern for WebsocketsProviderV2 if it isn't already connected

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch 2 times, most recently from 7437a3f to 6a8d428 Compare October 5, 2023 17:36

reedsa reviewed Oct 5, 2023

View reviewed changes

docs/providers.rst Outdated Show resolved Hide resolved

web3/_utils/module_testing/persistent_connection_provider.py Outdated Show resolved Hide resolved

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from f67ac1b to b2ec70f Compare October 5, 2023 21:18

fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 5, 2023

Changes from comments on PR ethereum#3116

bdd83a7

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from 4466efa to 9d275c4 Compare October 5, 2023 22:31

fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 6, 2023

Changes from comments on PR ethereum#3116

a9fcd00

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from 9d275c4 to ed1947e Compare October 6, 2023 15:08

fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 6, 2023

Changes from comments on PR ethereum#3116

4afe5a8

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from ed1947e to 3ab234e Compare October 6, 2023 15:39

fselmo added a commit to fselmo/web3.py that referenced this pull request Oct 6, 2023

Changes from comments on PR ethereum#3116

1081e80

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from 3ab234e to bbe0456 Compare October 6, 2023 15:47

fselmo added 4 commits October 6, 2023 10:10

Add unit tests for WebsocketProviderV2 make request

c89bde7

- Test that we cache any undesired messages until we find the one we want and return it. - Test that we return the cached response if in cache without ever calling recv()

Tweak formatters comparison % match; add syncing tests

321090e

- Add tests for ``syncing`` eth_subscribe requests - Tweak formatters keyset match to 75% match. Across all eth_subscribe requests, the keys are not close to being similar so we can afford a more relaxed check here.

Changes from comments on PR ethereum#3116

1fe663c

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from bbe0456 to 677d7f2 Compare October 6, 2023 16:10

reedsa reviewed Oct 6, 2023

View reviewed changes

web3/providers/websocket/websocket_v2.py Outdated Show resolved Hide resolved

web3/providers/websocket/websocket_v2.py Show resolved Hide resolved

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from 677d7f2 to af9adc8 Compare October 6, 2023 17:53

fselmo force-pushed the properly-apply-formatting-middleware-to-subscriptions branch from af9adc8 to e6995a8 Compare October 6, 2023 17:54

pacrob approved these changes Oct 6, 2023

View reviewed changes

reedsa reviewed Oct 6, 2023

View reviewed changes

reedsa approved these changes Oct 6, 2023

View reviewed changes

fselmo merged commit f55177b into ethereum:main Oct 6, 2023

fselmo deleted the properly-apply-formatting-middleware-to-subscriptions branch October 6, 2023 20:58

fselmo added a commit that referenced this pull request Oct 6, 2023

relevant newsfragments for #3116

0b68987

fselmo added a commit that referenced this pull request Oct 6, 2023

Changes from comments on PR #3116

ef022ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WebsocketProviderV2 updates + formatting middleware bugfix #3116

WebsocketProviderV2 updates + formatting middleware bugfix #3116

Uh oh!

fselmo commented Oct 4, 2023 •

edited

Loading

Uh oh!

reedsa left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pacrob left a comment

Uh oh!

reedsa Oct 6, 2023

Uh oh!

fselmo Oct 6, 2023

Uh oh!

reedsa Oct 6, 2023

Uh oh!

fselmo Oct 6, 2023

Uh oh!

reedsa left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-                if self.call_timeout and self.call_timeout <= 20
-                else 20
+                if self.call_timeout and self.call_timeout <= DEFAULT_PERSISTENT_CONNECTION_TIMEOUT
+                else DEFAULT_PERSISTENT_CONNECTION_TIMEOUT

WebsocketProviderV2 updates + formatting middleware bugfix #3116

WebsocketProviderV2 updates + formatting middleware bugfix #3116

Uh oh!

Conversation

fselmo commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong?

Todo:

Cute Animal Picture

Uh oh!

reedsa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pacrob left a comment

Choose a reason for hiding this comment

Uh oh!

reedsa Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

fselmo Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

reedsa Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

fselmo Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

reedsa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fselmo commented Oct 4, 2023 •

edited

Loading