Make default timeout configurable #192

skewty · 2023-02-12T17:29:23Z

fixes #116

NOTE: timeout field of Client._wait_for has type float but default value all over code is int.

fixes #116

for more information, see https://pre-commit.ci

codecov · 2023-02-12T17:31:20Z

Codecov Report

Merging #192 (15e1675) into main (d12fed7) will decrease coverage by 0.3%.
The diff coverage is 71.4%.

@@           Coverage Diff           @@
##            main    #192     +/-   ##
=======================================
- Coverage   89.9%   89.7%   -0.3%     
=======================================
  Files          6       6             
  Lines        729     734      +5     
  Branches     154     156      +2     
=======================================
+ Hits         656     659      +3     
  Misses        48      48             
- Partials      25      27      +2

Impacted Files	Coverage Δ
asyncio_mqtt/client.py	`81.5% <71.4%> (-0.3%)`	⬇️

JonathanPlasse · 2023-02-12T18:20:22Z

Thanks for your pull-request.
With the numeric tower, you only need float as it is valid to pass int to float.

frederikaalund

Hi Scott, thanks for opening this pull request. 👍 Let me have a look. :)

Great idea to add "layered" timeout (use argument if given; otherwise, use instance default). 👍

Overall, it looks good. :) I only have minor comments (see the review). Let me know if anything is unclear, or, if you have questions.

~Frederik

frederikaalund · 2023-02-13T08:26:58Z

asyncio_mqtt/client.py

@@ -250,6 +250,7 @@ def __init__(  # noqa: C901
        will: Will | None = None,
        clean_session: bool | None = None,
        transport: str = "tcp",
+        timeout: int | float = 10,


I propose that we use timeout: float | None = None. Note that int is redundant as per PEP 484:

[...] when an argument is annotated as having type float, an argument of type int is acceptable; [...]

Thanks to @JonathanPlasse for pointing that out. 👍

Note that we use None to mean "Give me the default value". It doesn't mean "no timeout". If you want "no timeout", use math.inf instead. :)

frederikaalund · 2023-02-13T08:27:54Z

asyncio_mqtt/client.py

@@ -360,6 +361,7 @@ def __init__(  # noqa: C901
        if socket_options is None:
            socket_options = ()
        self._socket_options = tuple(socket_options)
+        self.timeout = timeout


Replace with:

if timeout is None: timeout = 10 self._timeout: float = timeout

I intentionally made timeout public. Did you intentionally change it to protected?

I guess that was out of habit. 😄 We can keep it public. 👍 Just make a comment about it like this:

# This property is intentionally public since it's safe to modify at any point during the lifetime # of this instance. Aka: There are no race conditions. self.timeout = timeout

Otherwise, I'll probably forget that it is "safe" and think it was left public by accident. 😅

frederikaalund · 2023-02-13T08:30:17Z

asyncio_mqtt/client.py

@@ -380,7 +382,7 @@ def _pending_calls(self) -> Generator[int, None, None]:
        yield from self._pending_unsubscribes.keys()
        yield from self._pending_publishes.keys()

-    async def connect(self, *, timeout: int = 10) -> None:
+    async def connect(self, *, timeout: int | float | None = None) -> None:


Good idea to have method-local overrides to timeout. 👍

I propose that we use timeout: float | None = None (see the previous comment).
Furthermore, I propose the following semantics (written in psedo-code here):

if timeout is None: "Use the instance default (`self._timeout`)" else: "Use the `timeout` argument"

frederikaalund · 2023-02-13T08:32:56Z

asyncio_mqtt/client.py

@@ -404,15 +406,19 @@ async def connect(self, *, timeout: int = 10) -> None:
        # See: https://github.com/eclipse/paho.mqtt.python/blob/v1.5.0/src/paho/mqtt/client.py#L1770
        except (OSError, mqtt.WebsocketConnectionError) as error:
            raise MqttError(str(error)) from None
-        await self._wait_for(self._connected, timeout=timeout)
+        await self._wait_for(
+            self._connected, timeout=self.timeout if timeout is None else timeout


Again, good job with the "layered" timeout (use argument if given; otherwise, use instance default). Since we use that everywhere, let's move it inside _wait_for itself (to remove redundancy).

Inside _wait_for we have:

if timeout is None: timeout = self._timeout # Note that asyncio uses `None` to mean "No timeout". We use `math.inf`. timeout_for_asyncio = None if timeout == math.inf else timeout try: return await asyncio.wait_for(fut, timeout=timeout_for_asyncio, **kwargs) except ...

frederikaalund · 2023-02-13T08:33:30Z

asyncio_mqtt/client.py

        rc = self._client.disconnect()
        # Early out on error
        if rc != mqtt.MQTT_ERR_SUCCESS:
            raise MqttCodeError(rc, "Could not disconnect")
        # Wait for acknowledgement
-        await self._wait_for(self._disconnected, timeout=timeout)
+        await self._wait_for(
+            self._disconnected, timeout=self.timeout if timeout is None else timeout


See previous comment about _wait_for.

frederikaalund · 2023-02-13T08:33:41Z

asyncio_mqtt/client.py

@@ -441,15 +447,17 @@ async def subscribe(
        ] = asyncio.Future()
        with self._pending_call(mid, callback_result, self._pending_subscribes):
            # Wait for callback_result
-            return await self._wait_for(callback_result, timeout=timeout)
+            return await self._wait_for(
+                callback_result, timeout=self.timeout if timeout is None else timeout


See previous comment about _wait_for.

frederikaalund · 2023-02-13T08:33:47Z

asyncio_mqtt/client.py

-            await self._wait_for(confirmation.wait(), timeout=timeout)
+            await self._wait_for(
+                confirmation.wait(),
+                timeout=self.timeout if timeout is None else timeout,


See previous comment about _wait_for.

frederikaalund · 2023-02-13T08:33:53Z

asyncio_mqtt/client.py

-            await self._wait_for(confirmation.wait(), timeout=timeout)
+            await self._wait_for(
+                confirmation.wait(),
+                timeout=self.timeout if timeout is None else timeout,


See previous comment about _wait_for.

frederikaalund · 2023-02-13T08:43:40Z

Credit where credit is due: Use of math.inf for "No timeout" inspired by the anyio API. :)

frederikaalund · 2023-06-08T20:22:59Z

Hi skewty, thanks for the changes. 👍 It looks good to me except for one bug: The user can't disable the timeout.

To allow the user to disable the timeout (by setting timeout=math.inf), I need this code inside _wait_for:

if timeout is None:
    timeout = self._timeout
# Note that asyncio uses `None` to mean "No timeout". We use `math.inf`.
timeout_for_asyncio = None if timeout == math.inf else timeout
try:
    return await asyncio.wait_for(fut, timeout=timeout_for_asyncio, **kwargs)
except ...

Does it make sense? Let me know if you want to make the change. Otherwise, I can commit this PR as is and push the change myself. 👍

skewty · 2023-06-08T21:55:20Z

Does it make sense? Let me know if you want to make the change. Otherwise, I can commit this PR as is and push the change myself. 👍

Please go ahead and merge what is here and make the fixes you want. 🎉
This code has been sitting around long enough.

frederikaalund · 2023-06-11T08:14:30Z

Please go ahead and merge what is here and make the fixes you want. 🎉
This code has been sitting around long enough.

Done! Thank you for your contribution to asyncio-mqtt. 👍

skewty and others added 2 commits February 12, 2023 09:24

Make default timeout configurable

907ce30

fixes #116

[pre-commit.ci] auto fixes from pre-commit.com hooks

ad55ed9

for more information, see https://pre-commit.ci

JonathanPlasse requested a review from frederikaalund February 12, 2023 18:20

frederikaalund requested changes Feb 13, 2023

View reviewed changes

code adjustments as per comments

15e1675

skewty closed this Jun 8, 2023

skewty reopened this Jun 8, 2023

frederikaalund merged commit bfdbbf2 into empicano:main Jun 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default timeout configurable #192

Make default timeout configurable #192

skewty commented Feb 12, 2023

codecov bot commented Feb 12, 2023 •

edited

Loading

JonathanPlasse commented Feb 12, 2023

frederikaalund left a comment

frederikaalund Feb 13, 2023 •

edited

Loading

frederikaalund Feb 13, 2023

skewty Feb 13, 2023

frederikaalund Feb 13, 2023

frederikaalund Feb 13, 2023

frederikaalund Feb 13, 2023 •

edited

Loading

frederikaalund Feb 13, 2023

frederikaalund Feb 13, 2023

frederikaalund Feb 13, 2023

frederikaalund Feb 13, 2023

frederikaalund commented Feb 13, 2023

frederikaalund commented Jun 8, 2023

skewty commented Jun 8, 2023

frederikaalund commented Jun 11, 2023

Make default timeout configurable #192

Make default timeout configurable #192

Conversation

skewty commented Feb 12, 2023

codecov bot commented Feb 12, 2023 • edited Loading

Codecov Report

JonathanPlasse commented Feb 12, 2023

frederikaalund left a comment

Choose a reason for hiding this comment

frederikaalund Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikaalund Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikaalund commented Feb 13, 2023

frederikaalund commented Jun 8, 2023

skewty commented Jun 8, 2023

frederikaalund commented Jun 11, 2023

codecov bot commented Feb 12, 2023 •

edited

Loading

frederikaalund Feb 13, 2023 •

edited

Loading

frederikaalund Feb 13, 2023 •

edited

Loading