Added caching to the async session in request.py and AsyncHTTPProvider #2254

dbfreem · 2021-12-15T02:48:33Z

What was wrong?

When I was researching the timeouts in the benchmark test I noticed the session is not cached in the request.py for Async aiohttp request. Mainly when I read this issue on the geth repo about multiple http sessions I realized this might be an issue.

Also, the aiohttp library recommends reusing sessions

I feel like as a follow up to this PR the request.py should be broken apart into two classes. One for Async and one for standard HTTP. This would better organize the code and make it more testable.

How was it fixed?

For the test I started with what was in the test_precached_session and modified it for the async request. I also used the pytest.mark.asyncio that I found else where in the project for async testing.
I also started with the code that was used in request.py and HTTPProvider for the caching and modified it for async.

I upgraded pytest-mock trying to get the Async Mocking to work for the aiohttp session with no success in python 3.6. Since I couldn't get the mock working on the async session.post I only tested the session caching and not the post call on the session in request.py. Since there didn't seem to be any conflicts with the upgrade as long as it was less than 3.3.0 I left it in this PR.

I did find an issue later on related to session caching in the web3.py issues list.
Related to Issue #2016

Todo:

Add entry to the release notes

Upgraded pytest-mock

dbfreem · 2021-12-15T03:03:13Z

@kclowes do you have any feedback on this PR. Am I on the right track?

kclowes · 2021-12-15T22:03:48Z

This looks good @dbfreem! Thanks for looking into this and for all the PRs!

On first glance, this is about what I would expect in regards to caching the session. I'd probably drop the pytest-mock dependency upgrade if you're not really using it in this PR. If you feel strongly about upgrading it in general, it probably should be in its own PR.

If you pull in the latest master, some of those integration test failures should be less flaky.

dbfreem · 2021-12-16T00:49:28Z

I removed the the pytest-mock upgrade and pulled the latest from master.

I love python and enjoy the web3 concept in general so just thought I would try to get involved.

…eem/web3.py into feature/async_session_cache

kclowes

Thanks for this PR @dbfreem and for responding to so many of our issues!

There is an existing bug with the current sync implementation where the LRU cache isn't thread safe (see here: #1847), and I wonder what it would take to make this async version thread safe right off the bat. It may be as easy as pulling in the threading library and using threading.Lock as a context manager, but I'm not sure. I am also fine putting that in a different PR (that either you or someone else can tackle, no pressure).

Other than that, I just had a few nitpicks! Thanks again!

kclowes · 2021-12-17T19:07:14Z

tests/core/utilities/test_request.py

+    request.cache_async_session(URI, session)
+    assert len(request._async_session_cache) == 1
+
+    # Make sure a request with a different URI addes another cached session


Suggested change

# Make sure a request with a different URI addes another cached session

# Make sure a request with a different URI adds another cached session

kclowes · 2021-12-17T19:08:09Z

tests/core/utilities/test_request.py

+    assert len(request._async_session_cache) == 1
+
+    # Make sure a request with a different URI addes another cached session
+    request.cache_async_session("{0}/test".format(URI), session)


Nitpick - we tend to prefer using f-strings:

Suggested change

request.cache_async_session("{0}/test".format(URI), session)

request.cache_async_session(f"{URI}/test", session)

dbfreem · 2021-12-17T22:20:15Z

I was actually wondering if issue #2253 was a thread safe issue on the standard http side.

I will take a look at your comments and thread safety.

dbfreem · 2021-12-18T12:38:44Z

So I haven't done a lot of threading before but based on what I say in cache.py plus reading up on python.org about threading lock I think this will work. Unfortunately, this is really hard to test with a unit test.

If you agree with this threading approach I can make the same change in another PR for #1847

kclowes

This looks good @dbfreem. Thank you for the work! I've been looking a little bit at how to test the threading, so I'll let you know if I see anything promising.

dbfreem · 2022-01-13T10:18:18Z

working on the test for this and did find a bug in the logic so working on fixing that.

dbfreem · 2022-01-14T10:59:39Z

This still needs a little work so don't merge this. I will take a look this weekend.

dbfreem · 2022-01-16T12:05:17Z

So here is where I am with this. After I started writing a test to test the thread safety stuff I realized that ClientSession.close is an async method and I needed to wrap it in an async/await. When I did this the callback from the cache stopped working. So I decided to write a simple caching class since the requirements here are not that complex.

SessionCache is a class backed by an OrderedDict that simply removes the FIFO value from the dict when it reaches it's preset capacity. I also moved the closing of the connection to the calling code in request.py . IMO that code is more capable of knowing the best way to handle the closing of the connection either async or not instead of the cache handling the closing.

One other side effect that came of this is that I am no longer able to reproduce the threading issue that was there before I changed to SessionCache. Before implementing SessionCache if I ran multiple thread inserting into the cache I would get weird errors when the callback was called. After implementing SessionCache I am no longer able to reproduce these. I did leave the thread lock in for now just for good measure. But we may be able to try SessionCache without the thread lock and get users to test it out if they have seen threading issues before.

I do see that lru-dict is used in cache.py so this doesn't remove the dependency on it but I do feel like the use case for the cache in request.py is pretty simple and SessionCache provides a lot more control over the cache.

Thoughts?? I can go another route if you all do not agree with the SessionCache class. I would need to figure out how to get lru-dict to call the async callback, which could have been user error on my end but I couldn't get that to work. If you do agree with the new approach I can move the request.Session cache over to SessionCache in another PR.

…async

dbfreem · 2022-01-16T19:45:12Z

Ok, final answer here. Once, I built the unit test for the AsyncHTTPProvider I realized that the session wasn't being cached since the cache_async_session method is async. I moved this to an async method on the AsyncHTTPProvider to fix this.

All these changes started from the fact that the ClientSession.close() is an asnyc method itself. When caching an item in a fixed sized cache the eviction of some items need to be handled when new items enter the cache. This means calling ClientSession.close() which again is async.

Also, on the new test_async_http_provider module I added. I got the http://mynode.local:8545 from the test_http_provider module.

@kclowes can you review this

kclowes · 2022-01-17T17:26:42Z

Thanks @dbfreem! One of us will take a look this week!

dbfreem · 2022-02-01T02:08:03Z

@pacrob or @kclowes I wanted to check in and see if I could get a review on this one.

kclowes · 2022-02-07T19:04:12Z

@dbfreem haven't forgotten this one. Will take a look! Thank you!

dbfreem · 2022-02-08T10:44:04Z

Thanks @kclowes. I know this turned into a big PR, sorry about that. It kind of spiraled once I realized that the session close for the aiohttp was an async method and it wasn't actually being closed the way I had it at first.

docs/providers.rst

web3/_utils/request.py

Co-authored-by: Paul Robinson <[email protected]>

dbfreem · 2022-02-24T01:02:20Z

@pacrob I committed your suggestions. Thanks

ethereum#2254) * Added caching to the async session in request.py and AsyncHTTPProvider

#2254) (#2363) * Added caching to the async session in request.py and AsyncHTTPProvider Co-authored-by: AlwaysData <[email protected]>

pacrob · 2022-02-24T23:50:21Z

Thanks for adding this, @dbfreem ! I added the _ myself just to cut cycle time. Otherwise, awesome work as usual 👍

dbfreem · 2022-02-24T23:54:32Z

No worries about the _ @pacrob and you are welcome.

DefiDebauchery · 2022-03-03T19:07:07Z

Just a note that I'm having trouble utilizing this. I typically run web3 within a pycord (fork of discord.py) bot, which until this change has been working without issue.

I applied request.py and async_rpc.py. The conflict may be coming from the session lock, but hard to really debug.. I turned on the logging module and it seems to fail after initializing 10 RPC instances (needed for different evm networks), only one of them (not even using CustomSession yet). Will dig more, but wanted to mention this in case anyone might have thoughts.

kclowes · 2022-03-03T19:15:47Z

@DefiDebauchery thanks for the report. Any chance you can open up a new issue with a minimum reproducible example?

Added caching to the async session in request.py and AsyncHTTPProvider

38f7e84

Upgraded pytest-mock

dbfreem changed the title ~~Added caching to the async session in request.py and AsyncHTTPProvider~~ WIP Added caching to the async session in request.py and AsyncHTTPProvider Dec 15, 2021

dbfreem added 2 commits December 15, 2021 19:18

reverted pytest-mock upgrade

92ceac1

Merge branch 'ethereum:master' into feature/async_session_cache

ee778ae

dbfreem changed the title ~~WIP Added caching to the async session in request.py and AsyncHTTPProvider~~ Added caching to the async session in request.py and AsyncHTTPProvider Dec 16, 2021

dbfreem marked this pull request as ready for review December 16, 2021 00:53

dbfreem added 2 commits December 15, 2021 20:51

removed pytest-mock upgrade from news

229533f

Merge branch 'feature/async_session_cache' of https://github.com/dbfr…

801638b

…eem/web3.py into feature/async_session_cache

kclowes reviewed Dec 17, 2021

View reviewed changes

dbfreem added 3 commits December 17, 2021 20:01

made small suggested changes

3cff06f

typo

00f2c67

added thread lock to the async session cache

ff12510

kclowes reviewed Jan 5, 2022

View reviewed changes

Merge branch 'ethereum:master' into feature/async_session_cache

b9dcffb

dbfreem added 2 commits January 14, 2022 05:57

removed lock around close since it was causing deadlocks

e7bbec5

accidently commited test that I was working on

097fa1b

dbfreem added 4 commits January 15, 2022 20:29

Added SessionCache class

3d9e77b

rearranged the cache code

eae163d

added one more test on new SessionCache

65b12f2

lint

f713776

dbfreem added 2 commits January 16, 2022 07:10

Had the wrong import not sure how that worked locally

9656ae8

updated docs

88ee5ac

dbfreem added 4 commits January 16, 2022 07:32

fixed updating of existing cache items

1720df3

lint problem again

e307c8b

slight changes to docs

4289e64

Moved caching of session in AsynHTTPProvider to a method since it is …

8bdcbcd

…async

dbfreem added 2 commits January 25, 2022 21:13

Merge branch 'ethereum:master' into feature/async_session_cache

503146d

Merge branch 'ethereum:master' into feature/async_session_cache

92a2b84

Merge branch 'ethereum:master' into feature/async_session_cache

e1ff8fe

pacrob reviewed Feb 23, 2022

View reviewed changes

docs/providers.rst Outdated Show resolved Hide resolved

pacrob reviewed Feb 23, 2022

View reviewed changes

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

dbfreem and others added 2 commits February 23, 2022 19:59

Update docs/providers.rst

40ca5f9

Co-authored-by: Paul Robinson <[email protected]>

Update web3/_utils/request.py

cf29678

Co-authored-by: Paul Robinson <[email protected]>

add _ for confusion reduction

2d3a275

pacrob merged commit eed1ae4 into ethereum:master Feb 24, 2022

pacrob pushed a commit to pacrob/web3.py that referenced this pull request Feb 24, 2022

Added caching to the async session in request.py and AsyncHTTPProvider (

34d782e

ethereum#2254) * Added caching to the async session in request.py and AsyncHTTPProvider

pacrob mentioned this pull request Feb 24, 2022

Added caching to the async session in request.py and AsyncHTTPProvide… #2363

Merged

1 task

pacrob pushed a commit that referenced this pull request Feb 24, 2022

Added caching to the async session in request.py and AsyncHTTPProvider (

444de58

#2254) (#2363) * Added caching to the async session in request.py and AsyncHTTPProvider Co-authored-by: AlwaysData <[email protected]>

DefiDebauchery mentioned this pull request Apr 26, 2022

Async concurrency regression with 5.29.0 #2446

Closed

fselmo mentioned this pull request Feb 9, 2024

Allow users to pass a session object to aiohttp #2016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added caching to the async session in request.py and AsyncHTTPProvider #2254

Added caching to the async session in request.py and AsyncHTTPProvider #2254

dbfreem commented Dec 15, 2021

dbfreem commented Dec 15, 2021

kclowes commented Dec 15, 2021

dbfreem commented Dec 16, 2021 •

edited

Loading

kclowes left a comment

kclowes Dec 17, 2021

kclowes Dec 17, 2021

dbfreem commented Dec 17, 2021

dbfreem commented Dec 18, 2021

kclowes left a comment

dbfreem commented Jan 13, 2022

dbfreem commented Jan 14, 2022

dbfreem commented Jan 16, 2022 •

edited

Loading

dbfreem commented Jan 16, 2022 •

edited

Loading

kclowes commented Jan 17, 2022

dbfreem commented Feb 1, 2022

kclowes commented Feb 7, 2022

dbfreem commented Feb 8, 2022

dbfreem commented Feb 24, 2022

pacrob commented Feb 24, 2022

dbfreem commented Feb 24, 2022

DefiDebauchery commented Mar 3, 2022

kclowes commented Mar 3, 2022

	# Make sure a request with a different URI addes another cached session
	# Make sure a request with a different URI adds another cached session

	request.cache_async_session("{0}/test".format(URI), session)
	request.cache_async_session(f"{URI}/test", session)

Added caching to the async session in request.py and AsyncHTTPProvider #2254

Added caching to the async session in request.py and AsyncHTTPProvider #2254

Conversation

dbfreem commented Dec 15, 2021

What was wrong?

How was it fixed?

Todo:

dbfreem commented Dec 15, 2021

kclowes commented Dec 15, 2021

dbfreem commented Dec 16, 2021 • edited Loading

kclowes left a comment

Choose a reason for hiding this comment

kclowes Dec 17, 2021

Choose a reason for hiding this comment

kclowes Dec 17, 2021

Choose a reason for hiding this comment

dbfreem commented Dec 17, 2021

dbfreem commented Dec 18, 2021

kclowes left a comment

Choose a reason for hiding this comment

dbfreem commented Jan 13, 2022

dbfreem commented Jan 14, 2022

dbfreem commented Jan 16, 2022 • edited Loading

dbfreem commented Jan 16, 2022 • edited Loading

kclowes commented Jan 17, 2022

dbfreem commented Feb 1, 2022

kclowes commented Feb 7, 2022

dbfreem commented Feb 8, 2022

dbfreem commented Feb 24, 2022

pacrob commented Feb 24, 2022

dbfreem commented Feb 24, 2022

DefiDebauchery commented Mar 3, 2022

kclowes commented Mar 3, 2022

dbfreem commented Dec 16, 2021 •

edited

Loading

dbfreem commented Jan 16, 2022 •

edited

Loading

dbfreem commented Jan 16, 2022 •

edited

Loading