FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Disable multi requests on timeout during multi-request by KaloyanNaumov · Pull Request #1759 · python-kasa/python-kasa · GitHub

Repository navigation

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .py  (4) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
5 changes: 4 additions & 1 deletion kasa/httpclient.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from __future__ import annotations

import asyncio
import builtins
import logging
import ssl
import time
Expand Down Expand Up @@ -144,7 +145,9 @@ async def post(
raise _ConnectionError(
f"Device connection error: {self._config.host}: {ex}", ex
) from ex
except (aiohttp.ServerTimeoutError, TimeoutError) as ex:
# TimeoutError is imported from kasa.exceptions and shadows the builtin,
# which is what aiohttp raises when ClientTimeout(total=...) expires.
except (aiohttp.ServerTimeoutError, builtins.TimeoutError) as ex:
raise TimeoutError(
"Unable to query the device, "
+ f"timed out: {self._config.host}: {ex}",
Expand Down
11 changes: 10 additions & 1 deletion kasa/protocols/smartprotocol.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,16 @@ async def _execute_multiple_query(
batch_name,
pf(smart_request),
)
response_step = await self._transport.send(smart_request)
try:
response_step = await self._transport.send(smart_request)
except TimeoutError as ex:
# P300 does not respond to some batched requests (e.g. both child
# list requests together) so disable batching. Batch size is never
# 1 here as that case sends single requests above.
Comment on lines +251 to +253

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this is P300 specific, so I wanted to cleanup the comment. Alas, I could not push to your branch.

I am merging this now to get it into the next release I am preparing.

self._multi_request_batch_size = 1
raise _RetryableError(
"Timeout during multi request, multi requests disabled"
) from ex
if debug_enabled:
if self._redact_data:
data = redact_data(response_step, REDACTORS)
Expand Down
98 changes: 98 additions & 0 deletions tests/protocols/test_smartprotocol.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import logging

import pytest
Expand All @@ -9,6 +10,7 @@
DeviceError,
KasaException,
SmartErrorCode,
TimeoutError,
)
from kasa.protocols.smartcamprotocol import SmartCamProtocol
from kasa.protocols.smartprotocol import SmartProtocol, _ChildProtocolWrapper
Expand Down Expand Up @@ -235,6 +237,102 @@ async def test_smart_device_multiple_request_non_json_decode_failure(
assert send_mock.call_count == 1


async def test_smart_device_multiple_request_timeout(
dummy_protocol: SmartProtocol, mocker: MockerFixture
) -> None:
"""Test that a timed out multi request disables batching and retries."""
requests = {}
mock_responses = []
for i in range(10):
method = f"get_method_{i}"
requests[method] = {"foo": "bar", "bar": "foo"}
mock_responses.append(
{"method": method, "result": {"great": "success"}, "error_code": 0}
)

send_mock = mocker.patch.object(
dummy_protocol._transport,
"send",
side_effect=[TimeoutError("Simulated timeout"), *mock_responses],
)
mocker.patch("asyncio.sleep")
dummy_protocol._multi_request_batch_size = 5
resp = await dummy_protocol.query(requests, retry_count=1)
assert dummy_protocol._multi_request_batch_size == 1
assert resp == {method: {"great": "success"} for method in requests}
# Call count should be the timed out batch + number of requests
assert send_mock.call_count == len(requests) + 1


async def test_smart_device_multiple_request_timeout_single_requests(
dummy_protocol: SmartProtocol, mocker: MockerFixture
) -> None:
"""Test that timeouts on single requests are still retried and raised."""
send_mock = mocker.patch.object(
dummy_protocol._transport,
"send",
side_effect=TimeoutError("Simulated timeout"),
)
mocker.patch("asyncio.sleep")
dummy_protocol._multi_request_batch_size = 1
with pytest.raises(TimeoutError):
await dummy_protocol.query(DUMMY_MULTIPLE_QUERY, retry_count=2)
assert dummy_protocol._multi_request_batch_size == 1
assert send_mock.call_count == 3


async def test_smart_device_multiple_request_child_lists_timeout(
dummy_protocol: SmartProtocol, mocker: MockerFixture
) -> None:
"""Test devices that do not respond to batched child list requests.

The P300(EU) 1.0.7 firmware never answers a multipleRequest containing both
get_child_device_component_list and get_child_device_list, while each
request sent on its own succeeds.
"""
child_list_methods = {"get_child_device_component_list", "get_child_device_list"}
results = {
"get_child_device_component_list": {
"child_component_list": [],
"start_index": 0,
"sum": 0,
},
"get_child_device_list": {
"child_device_list": [],
"start_index": 0,
"sum": 0,
},
}

async def _send(request: str) -> dict:
req = json.loads(request)
if req["method"] != "multipleRequest":
return {"result": results[req["method"]], "error_code": 0}
methods = {r["method"] for r in req["params"]["requests"]}
if child_list_methods <= methods:
raise TimeoutError("Simulated timeout")
return {
"result": {
"responses": [
{"method": m, "result": results[m], "error_code": 0}
for m in methods
]
},
"error_code": 0,
}

send_mock = mocker.patch.object(
dummy_protocol._transport, "send", side_effect=_send
)
mocker.patch("asyncio.sleep")
assert dummy_protocol._multi_request_batch_size == 5
resp = await dummy_protocol.query(dict.fromkeys(child_list_methods))
assert resp == results
assert dummy_protocol._multi_request_batch_size == 1
# The timed out batch + one single request per method
assert send_mock.call_count == 3


async def test_childdevicewrapper_unwrapping(
dummy_protocol: SmartProtocol, mocker: MockerFixture
) -> None:
Expand Down
9 changes: 9 additions & 0 deletions tests/test_httpclient.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import builtins
import re

import aiohttp
Expand Down Expand Up @@ -35,6 +36,13 @@
TimeoutError,
"Unable to query the device, timed out: ",
),
# aiohttp raises the builtin TimeoutError when ClientTimeout(total=...)
# expires after the connection is established.
(
builtins.TimeoutError(),
TimeoutError,
"Unable to query the device, timed out: ",
),
(Exception(), KasaException, "Unable to query the device: "),
(
aiohttp.ServerFingerprintMismatch(b"exp", b"got", "host", 1),
Expand All @@ -47,6 +55,7 @@
"ClientOSError",
"ServerTimeoutError",
"TimeoutError",
"BuiltinTimeoutError",
"Exception",
"ServerFingerprintMismatch",
),
Expand Down

Back | FazBrowse Home | New Git URL