Repository: bachya/regenmaschine (language: python)

Issue:
Add "Connection: close" to all requests
**Describe what the PR does:**

[Home Assistant's RainMachine integration was throwing lots of weird exceptions](https://github.com/home-assistant/home-assistant/issues/23650). At regular intervals, we'd see logs like this:

```
2019-05-03 04:31:12 ERROR (MainThread) [homeassistant.components.rainmachine.switch] Unable to update info for zone "60e32719b6cf_zone_6": Error requesting data from 192.168.1.100: None
```

It appears that after some amount of time, the RainMachine forcibly closes the HTTP socket that `regenmaschine` uses; as a result, that request fails and it takes _another_ request to establish a new socket. This PR modifies the client so that every request as the `Connection: close` header, which will force the connection to close each time. We might miss out on some tiny speed increases by recreating the connection each time, but it shouldn't be noticeable.

**Does this fix a specific issue?**

N/A
  
**Checklist:**

- [x] Run tests and ensure 100% code coverage: `make coverage` (after running `make init`)
- [x] Ensure you have no linting errors: `make lint` (after running `make init`)
- [x] Ensure you have typed your code correctly: `make typing` (after running `make init`)



Discussion / hints:
# [Codecov](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=h1) Report
> Merging [#41](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=desc) into [dev](https://codecov.io/gh/bachya/regenmaschine/commit/c6a59fc2e64c3377e0fe969b1e8e3423789d2447?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/bachya/regenmaschine/pull/41/graphs/tree.svg?width=650&token=uW9kdau6Bd&height=150&src=pr)](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=tree)

```diff
@@         Coverage Diff         @@
##            dev    #41   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         4      4           
  Lines       153    153           
===================================
  Hits        153    153
```


| [Impacted Files](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [regenmaschine/client.py](https://codecov.io/gh/bachya/regenmaschine/pull/41/diff?src=pr&el=tree#diff-cmVnZW5tYXNjaGluZS9jbGllbnQucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=footer). Last update [c6a59fc...d803360](https://codecov.io/gh/bachya/regenmaschine/pull/41?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Fix patch:
diff --git a/.coveragerc b/.coveragerc
index f56e706..4936849 100644
--- a/.coveragerc
+++ b/.coveragerc
@@ -1,2 +1,6 @@
 [run]
 source = regenmaschine
+
+[report]
+exclude_lines =
+    assert False
diff --git a/regenmaschine/client.py b/regenmaschine/client.py
index 8a27cc4..15b9a7d 100644
--- a/regenmaschine/client.py
+++ b/regenmaschine/client.py
@@ -5,7 +5,7 @@
 from typing import Any, Dict, Optional, cast
 
 from aiohttp import ClientSession, ClientTimeout
-from aiohttp.client_exceptions import ClientError
+from aiohttp.client_exceptions import ClientError, ServerDisconnectedError
 import async_timeout
 
 from regenmaschine.controller import Controller, LocalController, RemoteController
@@ -57,7 +57,6 @@ async def _request(
             raise TokenExpiredError("Long-lived access token has expired")
 
         kwargs.setdefault("headers", {})
-        kwargs["headers"]["Connection"] = "close"
         kwargs["headers"]["Content-Type"] = "application/json"
 
         kwargs.setdefault("params", {})
@@ -74,23 +73,56 @@ async def _request(
         assert session
 
         try:
-            async with async_timeout.timeout(self._request_timeout):
-                async with session.request(method, url, ssl=ssl, **kwargs) as resp:
-                    resp.raise_for_status()
-                    data = await resp.json(content_type=None)
-                    _raise_for_remote_status(url, data)
+            # Only try 2x for ServerDisconnectedError to comply with the RFC
+            # https://datatracker.ietf.org/doc/html/rfc2616#section-8.1.4
+            for attempt in range(2):
+                try:
+                    return await self._request_with_session(
+                        session, method, url, ssl, **kwargs
+                    )
+                except ServerDisconnectedError as err:
+                    # The HTTP/1.1 spec allows the device to close the connection
+                    # at any time. aiohttp raises ServerDisconnectedError to let us
+                    # decide what to do. In this case we want to retry as it likely
+                    # means the connection was stale and the server closed it on us.
+                    if attempt == 0:
+                        continue
+                    raise RequestError(
+                        f"Error requesting data from {url} ({err}))"
+                    ) from err
+
+        finally:
+            if not use_running_session:
+                await session.close()
+
+        assert False, "unreachable"  # https://github.com/python/mypy/issues/8964
+
+    async def _request_with_session(
+        self,
+        session: ClientSession,
+        method: str,
+        url: str,
+        ssl: bool,
+        **kwargs: Dict[str, Any],
+    ) -> Dict[str, Any]:
+        """Make a request with a session."""
+        try:
+            async with async_timeout.timeout(self._request_timeout), session.request(
+                method, url, ssl=ssl, **kwargs
+            ) as resp:
+                resp.raise_for_status()
+                data = await resp.json(content_type=None)
+                _raise_for_remote_status(url, data)
+        except ServerDisconnectedError:
+            raise
         except ClientError as err:
             if "401" in str(err):
                 raise TokenExpiredError("Long-lived access token has expired") from err
             raise RequestError(f"Error requesting data from {url}") from err
         except asyncio.TimeoutError as err:
-            raise RequestError(f"Timeout during request: {url}") from err
-        finally:
-            if not use_running_session:
-                await session.close()
+            raise RequestError(f"Error requesting data from {url} ({err}))") from err
 
         _LOGGER.debug("Data received for %s: %s", url, data)
-
         return cast(Dict[str, Any], data)
 
     async def load_local(  # pylint: disable=too-many-arguments


Test patch:
diff --git a/tests/test_client.py b/tests/test_client.py
index b0c86ba..988c175 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -280,3 +280,43 @@ async def test_token_expired_implicit_exception(authenticated_local_client):
                     hours=1
                 )
                 await controller._request("get", "random/endpoint")
+
+
+@pytest.mark.asyncio
+async def test_retry_only_once_on_server_disconnected(
+    aresponses, authenticated_local_client
+):
+    """Test we retry on server disconnected."""
+    async with authenticated_local_client:
+        authenticated_local_client.add(
+            f"{TEST_HOST}:{TEST_PORT}",
+            "/api/4/restrictions/raindelay",
+            "get",
+            aresponses.Response(
+                text=load_fixture("restrictions_raindelay_response.json"), status=200
+            ),
+        )
+
+        async with aiohttp.ClientSession() as session:
+            client = Client(session=session)
+            await client.load_local(TEST_HOST, TEST_PASSWORD, port=TEST_PORT, ssl=False)
+            controller = next(iter(client.controllers.values()))
+            patcher = None
+
+            def _raise_and_stop_patch(*args, **kwargs):
+                nonlocal patcher
+                patcher.stop()
+                raise aiohttp.ServerDisconnectedError
+
+            patcher = mock.patch.object(
+                session, "request", side_effect=_raise_and_stop_patch
+            )
+            patcher.start()
+
+            data = await controller.restrictions.raindelay()
+            assert data["delayCounter"] == -1
+
+            with pytest.raises(RequestError), mock.patch.object(
+                session, "request", side_effect=aiohttp.ServerDisconnectedError
+            ):
+                await controller.restrictions.raindelay()