ethereum / web3.py

A python interface for interacting with the Ethereum blockchain and ecosystem.
http://web3py.readthedocs.io
MIT License
4.89k stars 1.67k forks source link

Strap a `RequestSessionManager` to each instance of http providers #3412

Closed fselmo closed 1 month ago

fselmo commented 1 month ago

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?

Todo:

Cute Animal Picture

20240528_215358~2

fselmo commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Yeah, I like that. More specific!

fselmo commented 1 month ago

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.