EnkaNetwork.py icon indicating copy to clipboard operation
EnkaNetwork.py copied to clipboard

Issues Identified in the `HTTPClient` Class

Open luoshuijs opened this issue 1 year ago • 2 comments

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.

luoshuijs avatar Sep 06 '23 01:09 luoshuijs

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

luoshuijs avatar Sep 06 '23 02:09 luoshuijs

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

luoshuijs avatar Sep 06 '23 03:09 luoshuijs