Add support for `async_simple_cache_middleware` #2579

fselmo · 2022-07-18T21:12:43Z

What was wrong?

Related to Issue #2578

Hard-coding an internal chain_id was considered at some point but ultimately we landed on adding eth_chainId to the list of whitelisted endpoints for simple_cache_middleware. The chain_id setter had made its way into the library already and some users had gotten around the multiple eth_chainId checks by using this setter, especially with AsyncEth.

Rather than introduce this setter back, this PR adds support to cache eth_chainId calls using the async_simple_cache_middleware with AsyncEth.

How was it fixed?

Async request caching support via async_simple_cache_middleware
Supports creating custom simple caching middleware via async_construct_simple_cache_middleware
Refactor SessionCache to a more generic SimpleCache within the caching _utils and use this cache class in the simple cache middleware as the default cache (for sync and async). Leave support for a simple dictionary cache as well.

Todo:

Add entry to the release notes

Cute Animal Picture

fselmo · 2022-07-18T21:58:40Z

web3/middleware/async_middleware/async_cache.py

+    make_request: Callable[[RPCEndpoint, Any], Any], async_w3: "Web3"
+):
+    middleware = await async_construct_simple_cache_middleware(
+        cache_class=cast(Type[Dict[Any, Any]], functools.partial(lru.LRU, 256)),


Interested in feedback on using LRU here vs anything else

The problem with LRU is that it's not thread safe, IIRC.

Correct, yeah... (#1847 for reference) currently we use LRU with sync as well. Not sure whether we want to add this in as a base and improve on both together or take care of it in this

I didn't plan on spending a ton of bandwidth on this this week so maybe that's the route to take if someone else wants to improve on this base in a separate PR before the next beta release?

We do use it with sync, but I believe the async implementation is currently thread safe. We do have some outstanding work to do with the session cache, though to get it to work just right. See #2446 and the corresponding PR (#2409 I think)

The other issue with using LRU with an async method is that it will not await the callback that it calls when it evicts an item. That was really the main reason that I wrote SessionCache in request.py for what it is worth. So if you are going to be using a callback with the above it will just return a coroutine (if I remember correctly) instead of executing the async callback.

If you really want to you could pull SessionCache out of request.py and put it somewhere in _utils then use that, but that would be a decent amount of work. The SessionCache itself is solid, the main issue still outstanding in request.py is how and where we await the close of an evicted async item.

fselmo · 2022-07-18T21:59:01Z

web3/middleware/async_middleware/async_cache.py

+
+        async def middleware(method: RPCEndpoint, params: Any) -> RPCResponse:
+            if method in rpc_whitelist:
+                with lock:


Interested on feedback in this main section of the thread locking

This is the exact issue I currently have in request.py with async and thread locking. The below await will release the execution of this method for something else to work while you are still in the lock. That will eventually cause a deadlock.

Yeah I think I get it a bit more now. Thanks for the explanation. I think the first step here is probably writing a good test that fails, with the situation you described, and work to make it not fail. I'll see if I can whip something up this week but I'm out for half of the week. Realistically probably won't get to this until next week.

I was able to use the POC in #2446 to pretty reliably reproduce the request.py issue, so maybe you could use something similar.

fselmo · 2022-07-18T22:05:37Z

This is a pretty rough copy of the sync with a few changes to make it work for async. Interested on async contributor feedback on whether the meat of the thread locking / simple cache setup could be improved upon. I just added this quickly so that we don't have to re-introduce a chainId setter just to take it out again. Caching seems more straightforward and is the direction we headed towards with sync.

cc: @dbfreem, @Eenae, @DefiDebauchery

fselmo · 2022-10-26T17:13:12Z

Factored SessionCache out as a more generic SimpleCache internal cache class. Used this as the default cache for the simple cache middleware while keeping dictionary-based cache support.

- Add async support for simple cache middleware - Refactor ``SessionCache`` as a more generic ``SimpleCache`` class to be used internally as a standardized cache where appropriate. - Use ``SimpleCache`` as the default cache for the simple cache middleware

- Include tests for dictionary-based cache and ``SimpleCache`` for simple cache middleware

kclowes

I had a couple nits, but looks good once we pull out the dictionary cache!

web3/_utils/request.py

pacrob

lgtm!

- Remove dictionary cache support altogether, for simple cache middleware, in favor of using the ``SimpleCache`` class. - Handle missing keys in ``SimpleCache`` a bit more gracefully than throwing a ``KeyError``, just return ``None`` if the key is not in the cache.

fselmo · 2022-10-31T23:14:51Z

I had a couple nits, but looks good once we pull out the dictionary cache!

Updated... I'm not quite sure how to add bullets to the feature newsfragment 🤔. Other than that this should be ready for a quick re-review w/ removal of the dictionary-based cache support.

kclowes

! Sweet!

fselmo changed the title ~~Add support for async_simple_cache_middleware~~ Add support for async_simple_cache_middleware Jul 18, 2022

fselmo force-pushed the async-simple-cache-middleware branch from 76ce961 to aae75ab Compare July 18, 2022 21:28

fselmo marked this pull request as ready for review July 18, 2022 21:36

fselmo requested review from kclowes and pacrob July 18, 2022 21:36

fselmo force-pushed the async-simple-cache-middleware branch from aae75ab to 191a9b2 Compare July 18, 2022 21:38

fselmo commented Jul 18, 2022

View reviewed changes

fselmo force-pushed the async-simple-cache-middleware branch 7 times, most recently from 81a34a8 to ef93744 Compare October 26, 2022 17:10

fselmo force-pushed the async-simple-cache-middleware branch from ef93744 to ae6274c Compare October 31, 2022 17:46

fselmo added 3 commits October 31, 2022 15:14

Update simple cache middleware tests

ad1ef92

- Include tests for dictionary-based cache and ``SimpleCache`` for simple cache middleware

Clean up unnecessary test teardowns via @pacrob

3f20e8c

fselmo force-pushed the async-simple-cache-middleware branch from fc53672 to 3f20e8c Compare October 31, 2022 21:14

kclowes approved these changes Oct 31, 2022

View reviewed changes

web3/_utils/request.py Outdated Show resolved Hide resolved

web3/_utils/request.py Outdated Show resolved Hide resolved

pacrob approved these changes Oct 31, 2022

View reviewed changes

fselmo force-pushed the async-simple-cache-middleware branch 3 times, most recently from 98a9b42 to b7a19c6 Compare October 31, 2022 23:00

fselmo force-pushed the async-simple-cache-middleware branch from b7a19c6 to 2e0e935 Compare October 31, 2022 23:13

fselmo requested a review from kclowes October 31, 2022 23:15

kclowes approved these changes Nov 2, 2022

View reviewed changes

fselmo merged commit 684fd0e into ethereum:master Nov 2, 2022

fselmo mentioned this pull request Nov 2, 2022

Please restore chain_id setter (#2376) #2578

Closed

fselmo deleted the async-simple-cache-middleware branch April 3, 2024 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `async_simple_cache_middleware` #2579

Add support for `async_simple_cache_middleware` #2579

fselmo commented Jul 18, 2022 •

edited

Loading

fselmo Jul 18, 2022

kclowes Jul 18, 2022

fselmo Jul 18, 2022 •

edited

Loading

kclowes Jul 18, 2022

dbfreem Jul 18, 2022

fselmo Jul 18, 2022

dbfreem Jul 18, 2022

fselmo Jul 19, 2022

dbfreem Jul 19, 2022

fselmo commented Jul 18, 2022 •

edited

Loading

fselmo commented Oct 26, 2022 •

edited

Loading

kclowes left a comment

pacrob left a comment

fselmo commented Oct 31, 2022

kclowes left a comment

Add support for async_simple_cache_middleware #2579

Add support for async_simple_cache_middleware #2579

Conversation

fselmo commented Jul 18, 2022 • edited Loading

What was wrong?

How was it fixed?

Todo:

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fselmo Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fselmo commented Jul 18, 2022 • edited Loading

fselmo commented Oct 26, 2022 • edited Loading

kclowes left a comment

Choose a reason for hiding this comment

pacrob left a comment

Choose a reason for hiding this comment

fselmo commented Oct 31, 2022

kclowes left a comment

Choose a reason for hiding this comment

Add support for `async_simple_cache_middleware` #2579

Add support for `async_simple_cache_middleware` #2579

fselmo commented Jul 18, 2022 •

edited

Loading

fselmo Jul 18, 2022 •

edited

Loading

fselmo commented Jul 18, 2022 •

edited

Loading

fselmo commented Oct 26, 2022 •

edited

Loading