-
Notifications
You must be signed in to change notification settings - Fork 406
Fix(httpx): Improve client cleanup and error handling #281
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
base: main
Are you sure you want to change the base?
Conversation
This commit addresses several issues in the HttpxLanguageModel to improve robustness and correct resource management: Remove Unreliable __del__ Cleanup: The __del__ method is not a reliable way to close an httpx.AsyncClient. It can fail if no event loop is running at garbage collection time, leading to ResourceWarning: unclosed transport errors. This finalizer has been removed. Add Explicit aclose() Method: An explicit async def aclose(self) method has been added. The __aexit__ method now calls await self.aclose(), making async with the primary, correct way to manage the client's lifecycle. Refine Retry Logic: The try...except block in complete was too broad and would incorrectly retry all exceptions, including JSON parsing errors on a successful 200 OK response. The logic is now split: It only retries on httpx.RequestError (like timeouts or connection errors) or specific transient HTTP status codes (429, 5xx). It now fails fast on non-retriable errors, such as failing to parse a successful response, preventing wasted retries. Implement Exponential Backoff: Retries now use an exponential backoff delay (self.retry_pause_seconds * (2 ** retry_count)) instead of a fixed pause. This is more effective for handling API rate limits (429) and temporary server unavailability (5xx).
|
@microsoft-github-policy-service agree |
|
|
||
| retry_count = 0 | ||
| while True: | ||
| response: httpx.Response | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this get hoisted out of the try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was pre-declared outside the try block (e.g., response: httpx.Response | None = None) to satisfy the static type checker (like Mypy) and to prevent potential UnboundLocalError bugs.
Here’s the simple explanation:
- Problem: The
responsevariable gets its value inside thetryblock (response = await ...). - Risk: If the
awaitcall fails and throws an exception, theresponsevariable never gets assigned. - Error: If any code after the
try...exceptblock tried to read theresponsevariable, it would crash with anUnboundLocalErrorbecause the variable doesn't exist.
By "hoisting" it and initializing it as None before the try block, we guarantee two things:
- The
responsevariable always exists within the loop's scope. - The type checker understands that the variable's type can be either
None(if it failed) orhttpx.Response(if it succeeded), which forces us to write safer code that checks if the response isNone.
| except httpx.RequestError as e: | ||
| if retry_count >= self.max_retry_attempts: | ||
| return Failure(str(e) or f"{repr(e)} raised from within internal TypeChat language model.") | ||
| return Failure(str(e) or f"{type(e).__name__} raised from within internal TypeChat language model.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was wrong with using repr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return Failure(str(e) or f"{type(e).__name__} raised from within internal TypeChat language model.") | |
| return Failure(f"Failure from internal TypeChat language model: `{repr(e)}`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing was wrong with using repr! In fact, it was used intentionally as a safe fallback to make the error messages more robust.
The pattern you saw, str(e) or repr(e), is a common and recommended practice in Python for exception handling.
Here’s a simple breakdown of why it's used:
-
str(e)- This gives the "human-friendly" error message.
- Example:
Connection timed out
-
repr(e)- This gives the "developer-friendly" representation of the exception object, including its type.
- Example:
httpx.ConnectTimeout('Connection timed out')
The Problem
Sometimes, an exception can be raised with an empty error message.
raise ValueError("")If an exception e like this is caught:
str(e)will be an empty string"".- If your code only used
Failure(str(e)), you would get a completely empty, useless error, making debugging impossible.
The Solution
The code str(e) or repr(e) cleverly solves this. It uses a Python "short-circuit" evaluation that means:
"Try to use the result of
str(e). If it'sNoneor an empty string"", then use the result ofrepr(e)instead."
This guarantees you always get a useful error message.
- Good Case:
str(e)is"Connection timed out". The code uses this. - Empty Case:
str(e)is"". The code falls back torepr(e)(e.g.,ValueError('')), which is much better than an empty string.
Co-authored-by: Daniel Rosenwasser <[email protected]>
Co-authored-by: Daniel Rosenwasser <[email protected]>
This commit addresses several issues in the
HttpxLanguageModelto improve robustness and correct resource management:Remove Unreliable
__del__Cleanup:The
__del__method is not a reliable way to close anhttpx.AsyncClient. It can fail if no event loop is running at garbage collection time, leading toResourceWarning: unclosed transporterrors. This finalizer has been removed.Add Explicit
aclose()Method:An explicit
async def aclose(self)method has been added. The__aexit__method now callsawait self.aclose(), makingasync withthe primary, correct way to manage the client's lifecycle.Refine Retry Logic:
The
try...exceptblock incompletewas too broad and would incorrectly retry all exceptions, including JSON parsing errors on a successful 200 OK response. The logic is now split:httpx.RequestError(like timeouts or connection errors) or specific transient HTTP status codes (429, 5xx).Implement Exponential Backoff:
Retries now use an exponential backoff delay (
self.retry_pause_seconds * (2 ** retry_count)) instead of a fixed pause. This is more effective for handling API rate limits (429) and temporary server unavailability (5xx).