Add Correct kwargs to AsyncHTTPProvider to allow graceful shutdown

[kclowes]

What was wrong?

The session wasn't being properly closed in the AsyncHTTPProvider if there was more than one w3 instance, and warnings were being emitted.

Closes #3524

How was it fixed?

Added an async context manager where needed.

Todo:

• Clean up commit history
• Add or update documentation related to these changes
• Add entry to the release notes

Cute Animal Picture

[pacrob]

🐑

[fselmo]

This looks like this closes the session on every call. Is that what's going on? I think this negates the async session cache altogether. We have some logic that will look in the cache for a session and if it's closed, we have to recreate and replace the stale session.

Can we instead introduce this logic somewhere on the provider's garbage collection? Or we could encourage explicit usage of a provider.disconnect() which does the proper cleanup. That way we can still utilize open session caching. We could even implement this as a context manager on the provider instead of on the session, so when the context closes, we properly handle the closing on all open sessions.

Thoughts there?

[fselmo]

I would think we have tests for making sure the session is re-used though... maybe I'm off here. But if we don't have these tests, we should add them here as well.

[fselmo]

I would think we have tests for making sure the session is re-used though... maybe I'm off here. But if we don't have these tests, we should add them here as well.

Looks like we don't have a test like this. This would be good to add. The following test on test_go_ethereum_http.py does fail and confirm we re-create the session every time with these changes. It passes on main 😅 .

@pytest.mark.asyncio
    async def test_async_http_provider_reuses_cached_session(self, async_w3) -> None:
        await async_w3.eth.get_block("latest")
        session_cache = async_w3.provider._request_session_manager.session_cache
        assert len(session_cache) == 1
        session = list(session_cache._data.values())[0]

        await async_w3.eth.get_block("latest")
        assert len(session_cache) == 1
        assert session == list(session_cache._data.values())[0]

[kclowes]

Aha, thanks for taking a look @fselmo. I think only the persistent connection providers have a disconnect method, I'll fix that here.

[alexferl]

@fselmo @kclowes so when I added my comment on the issue #3524, I took a quick look at the code a bit, to see if there was a .close() method or async context manager lifecycle methods on AsyncWeb3. Obviously there isn't a close() method, but the async context manager lifecycle methods are also limited to being used with persistent providers as you can see here. Unfortunately, that means that there's no quick fix to allow adding lifecycle methods with non-persistent providers without a bit of refactoring so that's why I came up with closing the session in the cache.

Perhaps having like a close_all_sessions() method on the HTTPSessionManager and then bubbling that up with a close() on AsyncHTTPProvider could be a simple solution?

[kclowes]

Yeah, I came to the same conclusion @alexferl :) This now adds a disconnect method to the AsyncHTTPProvider, which will close all sessions and clear the cache. I'll add an issue to track down why exactly we need the timeout, but this is slightly better for now, I think.

[kclowes]

Okay, all fixed. The timeout is actually linked to in the docs, but this issue had an even better fix. Thanks for the assist @fselmo!

[fselmo]

lgtm!