Strap a RequestSessionManager to each instance of http providers

[fselmo]

What was wrong?

Though #3265 is not reproducible, this hopefully closes #3265 as there should be no more conflicts across separate instances due to unique instantiation of session managing per provider.

How was it fixed?

• Use a unique session manager for each instance of HTTPProvider, and AsyncHTTPProvider, by moving all the shared session cache logic into a class and strapping it to each provider instance.
• Update tests related to session caching
• Update docs and update the typing for request_retry_configuration along the way (Optional)

Todo:

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

Cute Animal Picture

[kclowes]

The idea of strapping this manager to a provider is cracking me up for some reason. I'm imagining ratchet straps 😆 Aaaanyway, this looks good to me! There wasn't really a good place to put this in the PR, but what do you think about moving the name of web3/_utils/request.py to something like web3/_utils/request_session_manager.py since the RequestSessionManager class is basically all the file has now? Otherwise I left some small nits, but nothing big.

[fselmo]

The idea of strapping this manager to a provider is cracking me up for some reason. I'm imagining ratchet straps 😆

😆

Aaaanyway, this looks good to me! There wasn't really a good place to put this in the PR, but what do you think about moving the name of web3/_utils/request.py to something like web3/_utils/request_session_manager.py since the RequestSessionManager class is basically all the file has now? Otherwise I left some small nits, but nothing big.

I think I'll still put the DEFAULT_REQUEST_TIMEOUT here and use it for the CCIP read http requests, which it doesn't matter what provider we are using, it still makes a http GET or POST as part of the spec. For that reason it might make sense to still keep this as request utils? Or should I move the class definition into request_session_manager.py and keep the minimal request.py as a space for adding more than just the DEFAULT_REQUEST_TIMEOUT?

[kclowes]

I think I'll still put the DEFAULT_REQUEST_TIMEOUT here and use it for the CCIP read http requests, which it doesn't matter what provider we are using, it still makes a http GET or POST as part of the spec. For that reason it might make sense to still keep this as request utils? Or should I move the class definition into request_session_manager.py and keep the minimal request.py as a space for adding more than just the DEFAULT_REQUEST_TIMEOUT?

Keeping DEFAULT_REQUEST_TIMEOUT in there sounds good to me, and therefore keeping this as request utils also probably makes the most sense? I wouldn't be opposed to splitting it out either though. Whatever you think!

[fselmo]

Keeping DEFAULT_REQUEST_TIMEOUT in there sounds good to me, and therefore keeping this as request utils also probably makes the most sense? I wouldn't be opposed to splitting it out either though. Whatever you think!

Actually, we have a _utils/http.py already... I'm thinking that since this is really only managing HTTP sessions, we rename the class to HTTPSessionManager, create a _utils/http_session_manager.py and put it there, and then move the default timeout to _utils/http.py as HTTP_REQUEST_TIMEOUT. Thoughts there?

[kclowes]

Yeah, I like that. More specific!

[fselmo]

Yeah, I like that. More specific!

Sweet. The changes were significant enough. Can I ask for one last pass starting at this commit before we call it done? I also added passing the timeout through to the respective sync and async cache_and_return_session() methods so we can use a more accurate value for triggering the session closing tasks in that method.

---

edit

Just kidding I had to update the commit hash, should be good now. I added the timeout check to the integrations tests as well.

[kclowes]

I left one comment about a None check, but other than that, looks good!