Async concurrency regression with 5.29.0 #2446
Comments
|
@DefiDebauchery any chance you can try what is in #2409 and see if you still have this problem. If it does, I can put in a PR to add this same change in the v5 branch. |
Unfortunately, this does not change the results. Different line numbers for error output, though: |
|
oop, thanks for the report @DefiDebauchery. Putting this near the top of my list to look at this week. |
|
@DefiDebauchery the default cache size is 8 so you have to bump your cache size to 14 or however many connections you're trying to make. I didn't look very closely but I don't think there's a straightforward API to do that right at the moment, but I can get to it in a little bit or review if someone else gets to it first. Edit: Here is where it gets set. I do think we need to allow users to configure this, but I don't really have a good idea about where to allow that customization. First thought is to allow a rpc = Web3(
AsyncHTTPProvider(
uri,
request_kwargs={'timeout': 10},
cache_size=20,
),
modules={'eth': AsyncEth, 'net': AsyncNet},
middlewares=[]
) Open to other ideas too! |
|
It seems awkward to have each individual provider instance explicitly set the cache size. Am I wrong that SessionCache is initialized once? So wouldn't it make more sense to instantiate it with the requested size, then pass the instance into the Provider? Pedantically, I guess it's 6 in one hand / half-dozen in the other - but any ability to modify this is certainly welcomed. |
|
@DefiDebauchery if you change the cache size does that fix the issue. You are correct the cache is only instantiated once across all providers so we need to think what is the best way to make that configurable. One short term fix could be to bump the default cache size to something higher like 20 or 30. It doesn't seem like that would be to much of a memory issue it would be more of an issue with holding all those connections open simultaneously. We could also maybe make request an instance on each provider instead of using it as a singletons but that seems way more intrusive and probably significant work. @kclowes thoughts? |
|
I think bumping the hard coded value to 20 or 30 seems like a good step for now. I'll get that PRed in a little bit. Right now, the API looks like: w3 = Web3(
AsyncHTTPProvider(
uri,
request_kwargs={'timeout': 10},
),
modules={'eth': AsyncEth, 'net': AsyncNet},
middlewares=[]
)
custom_session = ClientSession() # If you want to pass in your own session
await w3.provider.cache_async_session(custom_session)@DefiDebauchery are you suggesting something like: session_cache = SessionCache(size=25)
w3 = Web3(
AsyncHTTPProvider(
uri,
request_kwargs={'timeout': 10},
session_cache,
),
modules={'eth': AsyncEth, 'net': AsyncNet},
middlewares=[]
)Tagging @fselmo for opinions too! |
|
@kclowes Either that, or allowing |
|
@DefiDebauchery I bumped the default cache size to 20 in v5.29.1 as a band-aid fix. I'm currently looking into what the actual fix might be. |
@kclowes this is the fix for the async cache bug. I am closing the connection outside of the lock. What was happening was that since the ClientSession.close() is async it was awaiting that method and passing off the execution to the next async task. Well the first task was still stuck inside of the thread lock and a dead lock was occurring. After stepping through here more I realized it is SUPER important for the session cache to be higher than the number of sessions needed. If it is not then it will continue to cycle sessions in and out of the cache. Once the await finally finishes the ClientSession.close() the session being returned may not even be in the cache anymore because it was evicted by a different task. This means there is still a race condition here where a session could be returned from the cache and then evicted by another task which then closes it before it can be used. I almost think we should programmatically set the cache size and emit a warning if the cache grows over a certain size and then have a second hard limit where it can't grow above. couple of things to think about is that there could be some sort of connection limit on the underlying OS that we might hit, and memory constrained apps might have issues. I also wonder if we should do something like this. It is not thread safe but are consumers going to mix threading and asyncio execution. If the consumer will be doing either threading on the blocking request or asyncio on the aiohttp session then maybe we should move to the above linked approach for the async cache. Thoughts?? |
|
@dbfreem I have not forgotten about this! I hope to get to a more thorough review of this comment and the outstanding session PR this week. I think we want to try and have it be threadsafe, but I need to spend some time thinking about how that might work 🤔 |
|
I will try to take a look this week as well @kclowes 👀 |
|
Resolved in #2409 for web3.py |
|
@dbfreem do you think raising the cache size to a reasonable higher value is enough for your needs or do you specifically want to be able to set the cache size? I think with PR #2409 and a reasonably high cache size 99% of cases should not have issues. Open to looking into configuring the cache size though, it just might take a back seat for a bit on our end. |
|
Being able to set the cache easily would be really helpful. Right now, cache size is 20, and at the moment, I'm running 16 concurrent endpoints to monitor the chains that our platform has a presence. This is set to increase within the next 6 months. Granted, I could hardcode a change, but being able to set or otherwise override this in 'userspace' would be better. |
|
Yeah I'd definitely say 20 is on the low side considering how little memory that takes up. I imagine increasing this number to 50 or above for now. We'd definitely welcome a PR for customizing the cache size otherwise we can make an issue to track and we can get to it when we have some more time as I see this as a nice-to-have feature. I'm all for making the library as customizable as possible as long as it makes sense to so I'd be onboard with that. |
|
closed by #2409 |
|
Side note @DefiDebauchery, cache size is going up to |
|
Ah, I forgot this is in |
|
closed by #2691 |
Uh oh!
There was an error while loading. Please reload this page.
What was wrong?
Async code with the AsyncHTTPProvider hangs in 5.29.0; previously worked fine in 5.28.0.
(Shoddy) code sample:
With 5.28.0, the output is as expected.
When updating to 5.29.0, the output hangs. When I abort the program, the error output references a session cache lock (edited for readability)
I do see that some SessionCache code is included in 5.29.0. My use case may be quite different than what was intended by the Session Cache and is likely related to my report in #2254 (ack, my apologies for not following up on that, @kclowes!).
The text was updated successfully, but these errors were encountered: