Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues Identified in the HTTPClient Class #51

Open
luoshuijs opened this issue Sep 6, 2023 · 2 comments
Open

Issues Identified in the HTTPClient Class #51

luoshuijs opened this issue Sep 6, 2023 · 2 comments

Comments

@luoshuijs
Copy link

luoshuijs commented Sep 6, 2023

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
Copy link
Author

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
Copy link
Author

luoshuijs commented Sep 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant