Skip to content
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.

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

retry(seconds=...) broke #368

Closed
tony opened this issue Apr 19, 2022 · 9 comments · Fixed by #372
Closed

retry(seconds=...) broke #368

tony opened this issue Apr 19, 2022 · 9 comments · Fixed by #372
Labels

Comments

@tony
Copy link
Member

tony commented Apr 19, 2022

Instead

import time
t = time.process_time()

while (time.process_time() - t) * 1000 < 2:
    # stuff happens
    time.sleep(0.1)

Find cross platform way to do timeout with signals

See also tmux-python/tmuxp#620

tony added a commit to tmux-python/tmuxp that referenced this issue Apr 19, 2022
@tony tony added this to tmuxp Apr 19, 2022
@tony tony added the bug label Apr 19, 2022
tony added a commit to tmux-python/tmuxp that referenced this issue Apr 19, 2022
@categulario
Copy link
Contributor

It turns out that this is the cause of tmux-python/tmuxp#704 (comment) . The current implementation of the retry() function is equivalent to return True.

Why is this function necessary? Maybe it can be replaced by a sleep(some_millis) in most cases. Seems strange needing to retry a block of code thousands of times until a condition meets. That makes me think (and taking a look at the places where it is used) that it can actually be accurately replaced by a while loop with the condition that breaks (which in fact would be equivalent since current implementation is a return True).

@categulario
Copy link
Contributor

Ok, I just found a case where a piece of code was tried 89 times before it succeeded, which was about 300 ms.

@categulario
Copy link
Contributor

categulario commented May 17, 2022

I came up with this function:

def retry(fun, secs=1):
    ini = time.time()

    while not fun():
        end = time.time()
        if end - ini >= secs:
            raise Exception('retry timed out')

which does the retrying thing accurately with the timeout, it is more or less clean to use (little change to the code) and reports when the retrying timed out:

    def f():
        session.server._update_windows()
        return w.name == "sh"

    retry(f)

    assert w.name == "sh"

what do you think? Of course it can read the timeout from the environment just like the code does right now

@tony
Copy link
Member Author

tony commented May 17, 2022

@categulario I'm still working and won't be able to focus on this in earnest until the weekend, but here's what i have to offer (to make it easier, maybe):

class WaitTimeout(Exception):
    pass


class WaitConditionCallbackProtocol(typing.Protocol):
    def __call__(self: Generic) -> bool:
        ...


def wait_for_condition(
    self,
    callback: WaitConditionCallbackProtocol,
    wait_time=0.5,
    interval=0.25,
    raises=False,
):
    if not callable(callback):
        raise TypeError("callback should be callable")
    time_spent = 0
    condition = False
    while condition:
        self.server.update_panes()
        if time_spent >= wait_time:
            if raises:
                raise WaitTimeout(f"Condition timed out after {time_spent}")
            break
        condition = callback(self)
        if condition is True:
            return True
        time.sleep(interval)
        time_spent += interval
    return False


class WaitableMixin:
    wait_for_condition = wait_for_condition


# In window.py (example):
class Window(..., WaitableMixin):
    ...

# In pane.py (example):
class Pane(..., WaitableMixin):
    ...

Usage:

>>> window.wait_for_condition(lambda w: w.window_name == "test")
raises WaitTimeout

>>> window.wait_for_condition(lambda w: w.window_name == "test", raises=False)
False

>>> window.wait_for_condition(lambda w: w.window_name == "test", timeout=5, interval=1)
After 5 seconds of checking every 1 second

@categulario
Copy link
Contributor

For an instant I thought this issue was about the libtmux.test function only, which is used exactly 10 times in testing code.

I understand that its usage can be extended to tmuxp code for cases like tmux-python/tmuxp#96 and that probably motivates this code. However I see a few shortcomings:

  • If it is available only as a mixin then using it requires using it from the class that implements the mixin. It might be that implementing it in Window and Pane is enough, it might be not. To me seems restrictive for no good reason and if it ever needs to be used when no window or pane is available then it leads its users to doing some weird tricks to have the function. Function that can very easily be just a function attached to nothing.
  • I think the calculation of time spent should be done with the time module, just because operations (like calling the callback) take time, so for the sake of unnecessary precision I would just call time() as in my proposal.
  • I think the function should rise by default, and be explicitly silenced. A condition that didn't meet after the given time is probably an error anyways. Knowing the reason of the error by default seems reasonable.
  • the while loop never enters, but that's probably a typo.

Things that I like:

  • the WaitTimeout error
  • the check for callable argument
  • sleeping a small interval between calls, although it should probably be around .1 second or less to have a more "real-time" effect. Waiting for 250 ms and failing to meet the condition implies the user waiting for half a second, which starts to feel like lag.

@tony
Copy link
Member Author

tony commented May 17, 2022

@categulario Would you like to make a new PR with a wait function, using the time module?

if not i can get around to it in this weekend or next weekend

i may not be around in the weekdays as much due to normal work deadlines

@categulario
Copy link
Contributor

categulario commented May 18, 2022

@categulario Would you like to make a new PR with a wait function w/o the time function, using the time module?

Sorry, I didn't understand that. Got confused between "with a wait function" and "without the time function"

@tony
Copy link
Member Author

tony commented May 18, 2022

@categulario I edited it. I'm just asking if you want to make PR with an example of what you meant here

@categulario
Copy link
Contributor

Just updated #372 with that

@tony tony closed this as completed in #372 May 20, 2022
tony added a commit that referenced this issue May 20, 2022
Fixes #368, as retry() in its current form is broke and won't work within a
`with` block. The function is deprecated and will be removed in 0.13.x. 0.12.x
will warn when using it.

retry_until() is now available and will either raise or return False if raise=False
is passed.

See also:
- tmux-python/tmuxp#620
- tmux-python/tmuxp#704 (comment)
@tony tony moved this to Done in tmuxp May 20, 2022
tony added a commit to tmux-python/tmuxp that referenced this issue May 21, 2022
tony added a commit that referenced this issue Aug 5, 2022
tony added a commit that referenced this issue Aug 5, 2022
tony added a commit that referenced this issue Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants