Timeout argument for run()

[martinhoyer]

Would it make sense to add an optional timeout argument to host.run?
In run_local it could be passed to p.communicate() and I imagine the other backends would have similar way to specify timeout.
Sorry if I'm missing something, I'm new to testinfra.

[gaellick]

see https://pytest-timeouts.readthedocs.io/en/latest/

[amaslenn]

From BaseBackend:

    def run(self, command: str, *args: str, **kwargs: Any) -> CommandResult:
        raise NotImplementedError

So you can pass whatever parameter you want to host.run(), but the implementation depends on each backend.

@martinhoyer do you want all backends support timeout mandatory and change signature to

def run(self, command: str, timeout: int, *args: str, **kwargs: Any) -> CommandResult

?

[martinhoyer]

@amaslenn Thanks for looking into it.
Looking at run_local(), it would have to be passed here:

        p = subprocess.Popen(
            cmd,
            shell=True,
            stdin=subprocess.PIPE,
            stdout=subprocess.PIPE,
            stderr=subprocess.PIPE,
+           timeout=timeout
        )

The *args are being used to construct the actual command being run if I understand it correctly.

[amaslenn]

@martinhoyer so you need only in BaseBackend::run_local(...)? This could look like this:

-    def run_local(self, command: str, *args: str) -> CommandResult:
+    def run_local(
+        self, command: str, *args: str, timeout: Optional[float] = None
+    ) -> CommandResult:
...
-        stdout, stderr = p.communicate()
+        stdout, stderr = p.communicate(timeout=timeout)

But will that do what you need? Backends like Ansible or SSH are using this function, but they won't pass timeout arg automatically, so changes in backends will also be needed. Overall interface might become unpredictable.

Yet timeout setting seems pretty useful.

@philpep what do you think, would such change make sense?

[martinhoyer]

@amaslenn well, personally, I'm only using local for now, but I do realize that it would should be implemented in other backends, as mentioned in comment 0. If it's not as straightforward to implement there, then we can survive without it :)

SSH though should have a same "communicate" timeout argument at least afaik.

[amaslenn]

SSH is slightly different, it uses -o ConnectTimeout=, not the timeout on Python level.

But anyway, this looks like a very useful feature. Yet maybe to keep the existing interface, BaseBackend should introduce a function like run_with_timeout. There are plenty of options here, let's wait for project owners to continue.

[martinhoyer]

I see, it's running ssh commands directly. You can just pass it to run_local there as well then.