Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ def __repr__(self):

async def _request(**request_params) -> httpx.Response:
async with httpx.AsyncClient(
verify=request_params.pop("verify", True)
verify=request_params.pop("verify", True),
timeout=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While setting timeout=None restores the previous behavior of requests and fixes the immediate httpx.ReadTimeout issue, it introduces a significant risk. Disabling timeouts can cause requests to hang indefinitely if the remote API is unresponsive, potentially leading to resource exhaustion and service instability.

Even though a follow-up is mentioned in the pull request description, I strongly recommend addressing this now by setting a generous default timeout (e.g., 10 minutes) instead of None. This would provide a crucial safety net. A truly indefinite timeout should be an explicit choice by the user of the tool, not the default behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While setting timeout=None restores the previous behavior of requests, it's risky to have no timeout as requests can hang indefinitely, potentially leading to resource exhaustion.

To make this more robust and prepare for a future change where the timeout is configurable (as mentioned in the PR description), I suggest reading the timeout from request_params. This way, RestApiTool can be updated to control the timeout value without needing to change this helper function again.

Using request_params.pop("timeout", None) will keep the current behavior of no timeout by default if no other changes are made, but makes the function extensible for a proper fix.

Suggested change
timeout=None,
timeout=request_params.pop("timeout", None),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please address this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the implementation based on the feedback:

  1. Changed to request_params.pop("timeout", httpx.Timeout(600.0)) — this makes the timeout configurable via request_params, so RestApiTool can control timeout without modifying _request() in the future.
  2. Instead of None (no timeout), I set a generous 10-minute default as a safety net to prevent indefinite hangs while still accommodating slow API responses.

) as client:
return await client.request(**request_params)
Loading