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

✨ Decide on Supporting Non-AsyncContextManager Usage in BaseClient #35

Open
luoshuijs opened this issue Jun 11, 2023 · 0 comments
Open
Labels
help wanted Extra attention is needed

Comments

@luoshuijs
Copy link
Contributor

We're currently considering whether to support non-AsyncContextManager usage in the BaseClient class. We've noted several potential issues associated with this, including the possibility of resource and memory leaks if BaseClient instances aren't properly shut down after use.

The main concerns are as follows:

Resource Leakage: HTTP clients typically open network connections to send requests. If a client isn't correctly closed, these connections could remain open, consuming system resources. Over time, if new clients are frequently created and not closed, this could lead to a gradual consumption of system resources, potentially causing performance issues or system errors.

Memory Leakage: Python's garbage collector usually releases memory occupied by an object when it's no longer referenced. However, if an object keeps references to system resources (like open files or network connections) and doesn't provide a method to release these resources, they might not be garbage collected, leading to memory leaks.

Application Errors: Some HTTP client libraries may throw an error when trying to destroy a client object that hasn't been correctly closed. This could cause application crashes or create hard-to-debug issues.

To avoid these problems, the best practice would be to always shut down BaseClient instances when they're no longer needed. Currently, we're using an AsyncContextManager to manage the lifecycle of our AsyncClient instances automatically, but we're considering whether we need to support non-AsyncContextManager usage as well.

We need to carefully consider the implications of this decision, as supporting non-AsyncContextManager usage could potentially introduce more complexity and opportunities for errors in our code. Please share your thoughts and insights on this matter.

@luoshuijs luoshuijs added the help wanted Extra attention is needed label Jun 11, 2023
@luoshuijs luoshuijs pinned this issue Jun 11, 2023
@luoshuijs luoshuijs changed the title Decide on Supporting Non-AsyncContextManager Usage in BaseClient Decide on Supporting Non-AsyncContextManager Usage in BaseClient Jun 11, 2023
@luoshuijs luoshuijs changed the title Decide on Supporting Non-AsyncContextManager Usage in BaseClient ✨ Decide on Supporting Non-AsyncContextManager Usage in BaseClient Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant