EnkaNetwork.py
EnkaNetwork.py copied to clipboard
Issues Identified in the `HTTPClient` Class
Flawed Loop Logic
https://github.com/mrwan200/EnkaNetwork.py/blob/728f5ca42299c315f52993323e823f97c27c2dbf/enkanetwork/http.py#L121
When I set RETRY_MAX
to 2, tries
only gets assigned the values 0 or 1.
>>> RETRY_MAX = 2
>>> list(range(RETRY_MAX))
[0, 1]
>>>
As a result, the following code tries > RETRY_MAX
will always be false
. Meaning the code block after the if
statement will never be executed.
https://github.com/mrwan200/EnkaNetwork.py/blob/728f5ca42299c315f52993323e823f97c27c2dbf/enkanetwork/http.py#L144-L145
Coroutine Unsafe
The main problem with this code is the management of self.__session
and self.__headers
. If two coroutines call the request
method at the same time, both might attempt to modify these variables, leading to undefined behavior.
https://github.com/mrwan200/EnkaNetwork.py/blob/728f5ca42299c315f52993323e823f97c27c2dbf/enkanetwork/http.py#L108-L119
Default Parameters
Consider not using ''
for empty value checks, but None
instead. In the __init__
method, you have a default timeout timeout: int = 5
, but if the timeout is None
or 0
, you set it to 10
.
https://github.com/mrwan200/EnkaNetwork.py/blob/728f5ca42299c315f52993323e823f97c27c2dbf/enkanetwork/http.py#L83-L86
My recommended improvement:
def __init__(self, *, key: Optional[str] = None, agent: Optional[str] = None, timeout: Optional[str] = None) -> None: # noqa
self.__session: aiohttp.ClientSession = MISSING
self.__headers: Dict = {}
self.__timeout = timeout or 10
# Init User Agent
if agent is not None:
Config.init_user_agent(agent)
if key is not None:
warnings.warn("'key' has deprecated.")
Concerning headers
I can't quite grasp the purpose of headers
here. I believe passing headers
directly to aiohttp.ClientSession
during initialization might be a more concise and intuitive approach.
https://github.com/mrwan200/EnkaNetwork.py/blob/728f5ca42299c315f52993323e823f97c27c2dbf/enkanetwork/http.py#L108-L112
Session Management
__session
is assigned and cleared across different methods. I'd suggest considering the use of an AsyncContextManager
for this purpose.
On Some Discussions
Timeout Handling
Regarding the timeout
exception handling I previously raised, I'm quite puzzled when looking at the code below. I've also checked the official documentation of aiohttp
but couldn't find any guidance on handling the timeout exceptions thrown by aiohttp
(which is also a reason I really dislike aiohttp
).
https://github.com/mrwan200/EnkaNetwork.py/blob/728f5ca42299c315f52993323e823f97c27c2dbf/enkanetwork/http.py#L151-L156
If you use httpx
and do not implement request retries, the refactored HTTPClient.request()
method becomes more readable and maintainable.
https://github.com/PaiGramTeam/PaiGram/blob/b4ef81dae69b6750f8eeb42ef424904f6710982d/utils/enkanetwork.py#L68-L112