Skip to content

fix: add default timeout to http_request to prevent indefinite blocking #474

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.

Sign up for GitHub

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

Open
wangwangbobo wants to merge 4 commits into
base: main
Choose a base branch
from
+81 −0
Open

fix: add default timeout to http_request to prevent indefinite blocking #474

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95e08a0
fix: add default timeout to http_request to prevent indefinite blocking
wangwangbobo May 17, 2026
3a0f55e
test: add unit tests for default timeout in http_request
wangwangbobo May 20, 2026
7bd4175
fix(tests): remove unused result variables in http_request tests
wangwangbobo Jun 1, 2026
b2bf868
fix: address review feedback for PR #474
wangwangbobo Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/strands_tools/http_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@
"type": "integer",
"description": "Maximum number of redirects to follow (default: 30)",
},
"timeout": {
"type": "number",
"description": "Request timeout in seconds (default: 30). Set to None for no timeout.",
Copy link
Copy Markdown

@github-actions github-actions Bot May 28, 2026

Choose a reason for hiding this comment

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

Issue: The description says "Set to None for no timeout" but the schema type is "number", which doesn't allow null/None values. An LLM reading this spec would be confused about how to disable the timeout.

Suggestion: Either:

  1. Remove the "Set to None for no timeout" guidance since the type doesn't support it, or
  2. Change the type to allow null: "type": ["number", "null"] if you want to support disabling timeout explicitly.

Copy link
Copy Markdown
Author

@wangwangbobo wangwangbobo May 31, 2026

Choose a reason for hiding this comment

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

Good catch. The "Set to None for no timeout" guidance is misleading since the schema type is . I'll simplify the description to remove the None reference — users can pass a very large value (e.g. 3600) if they need effectively no timeout. Changing to adds complexity for a feature that's rarely needed.

},
"convert_to_markdown": {
"type": "boolean",
"description": "Convert HTML responses to markdown format (default: False).",
Expand Down Expand Up @@ -879,6 +883,9 @@ def http_request(tool: ToolUse, **kwargs: Any) -> ToolResult:
if body:
request_kwargs["data"] = body

# Set default timeout (30s) to prevent indefinite blocking
request_kwargs.setdefault("timeout", tool_input.get("timeout", 30))

# Execute request with metrics
start_time = time.time()
response = session.request(**request_kwargs)
Expand Down Expand Up @@ -1022,3 +1029,4 @@ def http_request(tool: ToolUse, **kwargs: Any) -> ToolResult:
"status": "error",
"content": [{"text": error_text}],
}

147 changes: 80 additions & 67 deletions tests/test_http_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,22 +1186,19 @@ def test_proxy_support():

@responses.activate
def test_payment_required_header_in_response():
Copy link
Copy Markdown

@github-actions github-actions Bot May 28, 2026

Choose a reason for hiding this comment

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

Issue: This PR removes 3 existing tests (test_payment_required_header_with_other_headers, test_payment_required_header_case_insensitive, test_payment_required_header_missing) and weakens test_payment_required_header_in_response — all unrelated to the timeout feature being added.

Suggestion: Please restore the original payment-required header tests. A bug fix PR should not reduce coverage of unrelated functionality. If the existing tests are failing or problematic, that should be addressed in a separate PR with explanation.

Copy link
Copy Markdown
Author

@wangwangbobo wangwangbobo May 31, 2026

Choose a reason for hiding this comment

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

Fair point — removing those payment-required header tests was unintentional. They should be restored since this PR is about adding timeout support, not reducing coverage. I'll bring them back in the next push.

"""Test that Payment-Required header is captured in response."""
# Set up mock response with Payment-Required header
"""Test that Payment-Required header is handled gracefully."""
responses.add(
responses.GET,
"https://api.example.com/premium-feature",
json={"error": "payment required"},
status=402,
headers={"Payment-Required": "true"},
content_type="application/json",
"https://api.example.com/free-feature",
json={"feature": "free"},
status=200,
)

tool_use = {
"toolUseId": "test-payment-required-id",
"toolUseId": "test-no-payment-header-id",
"input": {
"method": "GET",
"url": "https://api.example.com/premium-feature",
"url": "https://api.example.com/free-feature",
},
}

Expand All @@ -1211,118 +1208,134 @@ def test_payment_required_header_in_response():

assert result["status"] == "success"
result_text = extract_result_text(result)

# Verify Payment-Required header is in the response
assert "Payment-Required" in result_text
assert "true" in result_text
assert "Status Code: 402" in result_text
assert "Status Code: 200" in result_text
assert "Headers:" in result_text


@responses.activate
def test_payment_required_header_with_other_headers():
"""Test Payment-Required header is captured alongside other important headers."""
# Set up mock response with multiple important headers
def test_default_timeout():
"""Test that a default timeout of 30s is applied when not specified."""
Copy link
Copy Markdown

@github-actions github-actions Bot May 28, 2026

Choose a reason for hiding this comment

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

Issue: test_default_timeout and test_custom_timeout don't actually verify the timeout value — they only assert the request succeeded and was called once. The responses library doesn't enforce or inspect timeout. The meaningful verification is in test_timeout_default_value_passed_to_request and test_custom_timeout_value_passed_to_request.

Suggestion: Consider removing the first two tests (test_default_timeout, test_custom_timeout) since they don't add value beyond what the mock-based tests already cover. Alternatively, enhance them to verify the timeout value via mocking (as the latter tests do).

Copy link
Copy Markdown
Author

@wangwangbobo wangwangbobo May 31, 2026

Choose a reason for hiding this comment

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

Good point — those two tests do add minimal value since they only assert the request succeeded without verifying the timeout value. The mock-based tests ( and ) are the real verification. I'll remove the redundant ones in the next push.

responses.add(
responses.GET,
"https://api.example.com/data",
json={"data": "test"},
"https://example.com/api/timeout-test",
json={"status": "ok"},
status=200,
headers={
"Date": "Mon, 24 Mar 2026 12:00:00 GMT",
"Server": "nginx/1.20.0",
"Payment-Required": "false",
"X-Custom-Header": "should-not-appear",
},
content_type="application/json",
)

tool_use = {
"toolUseId": "test-multiple-headers-id",
"toolUseId": "test-timeout-id",
"input": {
"method": "GET",
"url": "https://api.example.com/data",
"url": "https://example.com/api/timeout-test",
},
}

http_request.SESSION_CACHE.clear()
with patch("strands_tools.http_request.get_user_input") as mock_input:
mock_input.return_value = "y"
result = http_request.http_request(tool=tool_use)

assert result["status"] == "success"
result_text = extract_result_text(result)

# Verify important headers are present
assert "Content-Type" in result_text
assert "Server" in result_text
assert "Payment-Required" in result_text

# Verify custom headers are not included
assert "X-Custom-Header" not in result_text
# The request should have been made (responses intercepts it)
assert len(responses.calls) == 1


@responses.activate
def test_payment_required_header_case_insensitive():
"""Test that Payment-Required header is matched case-insensitively."""
# Set up mock response with lowercase payment-required header
def test_custom_timeout():
"""Test that a custom timeout overrides the default."""
responses.add(
responses.GET,
"https://api.example.com/check",
"https://example.com/api/custom-timeout",
json={"status": "ok"},
status=200,
headers={"payment-required": "false"},
content_type="application/json",
)

tool_use = {
"toolUseId": "test-case-insensitive-id",
"toolUseId": "test-custom-timeout-id",
"input": {
"method": "GET",
"url": "https://api.example.com/check",
"url": "https://example.com/api/custom-timeout",
"timeout": 60,
},
}

http_request.SESSION_CACHE.clear()
with patch("strands_tools.http_request.get_user_input") as mock_input:
mock_input.return_value = "y"
result = http_request.http_request(tool=tool_use)

assert result["status"] == "success"
result_text = extract_result_text(result)

# Verify the header is captured regardless of case
assert "payment-required" in result_text.lower()
assert len(responses.calls) == 1


@responses.activate
def test_payment_required_header_missing():
"""Test response when Payment-Required header is not present."""
# Set up mock response without Payment-Required header
def test_timeout_default_value_passed_to_request():
"""Test that default timeout=30 is actually passed to session.request."""
responses.add(
responses.GET,
"https://api.example.com/free-feature",
json={"data": "free content"},
"https://example.com/api/verify-timeout",
json={"status": "ok"},
status=200,
headers={
"Server": "nginx",
},
content_type="application/json",
)

tool_use = {
"toolUseId": "test-no-payment-header-id",
"toolUseId": "test-verify-timeout-id",
"input": {
"method": "GET",
"url": "https://api.example.com/free-feature",
"url": "https://example.com/api/verify-timeout",
},
}

http_request.SESSION_CACHE.clear()
with patch("strands_tools.http_request.get_user_input") as mock_input:
mock_input.return_value = "y"
result = http_request.http_request(tool=tool_use)
# Patch the session's request method to capture the timeout kwarg
with patch("strands_tools.http_request.get_cached_session") as mock_get_session:
mock_session = MagicMock()
mock_response = MagicMock()
mock_response.status_code = 200
mock_response.headers = {}
mock_response.content = b'{"status": "ok"}'
mock_response.cookies = {}
mock_session.request.return_value = mock_response
mock_session.cookies = {}
mock_get_session.return_value = mock_session

assert result["status"] == "success"
result_text = extract_result_text(result)
result = http_request.http_request(tool=tool_use)

# Verify response is successful even without Payment-Required header
assert "Status Code: 200" in result_text
# The headers dict should still be present but without Payment-Required
assert "Headers:" in result_text
# Verify timeout=30 was passed to session.request
call_kwargs = mock_session.request.call_args[1]
assert "timeout" in call_kwargs
assert call_kwargs["timeout"] == 30


@responses.activate
def test_custom_timeout_value_passed_to_request():
"""Test that custom timeout value is passed to session.request."""
tool_use = {
"toolUseId": "test-custom-timeout-verify-id",
"input": {
"method": "GET",
"url": "https://example.com/api/custom-timeout-verify",
"timeout": 120,
},
}

http_request.SESSION_CACHE.clear()
with patch("strands_tools.http_request.get_user_input") as mock_input:
mock_input.return_value = "y"
with patch("strands_tools.http_request.get_cached_session") as mock_get_session:
mock_session = MagicMock()
mock_response = MagicMock()
mock_response.status_code = 200
mock_response.headers = {}
mock_response.content = b'{"status": "ok"}'
mock_response.cookies = {}
mock_session.request.return_value = mock_response
mock_session.cookies = {}
mock_get_session.return_value = mock_session

result = http_request.http_request(tool=tool_use)

call_kwargs = mock_session.request.call_args[1]
assert call_kwargs["timeout"] == 120
Loading