Skip to main content
1 of 6
Dan Oberlam
  • 8k
  • 2
  • 33
  • 74

I like all of Ev. Kounis' answer, so I'll add some higher level details.

Let it be truth-y

Right now you aren't strictly requiring func to return True or False, just something truth-y or false-y. This is fine, and Pythonic. I think you would benefit from pointing that out in a docstring (which you should also write). Additionally, it may be worthwhile to return the actual value of func(*func_args) instead of True or False; this way you can actually get a value from the function if you wanted to take advantage of the truth-yness of the value.

Better kwargs support

You also don't allow any keyword arguments to be passed to the function - the function might support them, so you should to. I would promote the two keyword arguments you pull out of kwargs to explicit keyword arguments, with appropriate defaults (5 in your case).

Exceptions

It's weird to me that this function does have any concept of retrying after exceptions. I wouldn't want you to do something like this, for what I hope are obvious reasons

for _ in range(retry_count):
    try:
        if func(*func_args):
            return True
    except:
        pass
    log.debug("wating for %s seconds before retrying again")
    sleep delay)

However, in many cases I suspect you would know what exceptions you might expect (for example, a network timeout or connectivity blip) and that you might want to be handled by the retrying framework. To this end, I think something like this could be nice:

def retry(func, retry_count=5, delay=5, allowed_exceptions=(), *args, **kwargs):
    for _ in range(retry_count):
        try:
            result = func(*func_args, **kwargs)
            if result: return result
        except Exception as e:
            if e not in allowed_exceptions:
                raise

This obviously isn't a complete implementation; I left out some of the other pieces you have, and it behaves oddly if it fails on the last iteration, but it should be enough to start.

Fancy stuff

I think we could also get more value from this if it was a decorator. Then consumers of a function don't even need to know if they want it to retry or not; they just call their function and see that it works, and whether or not it was retried becomes irrelevant. Don't forget to use functools.wraps to preserve metadata.

import functools

def retry(retry_count=5, delay=5, allowed_exceptions=()):
    def decorator(f):
        @functools.wraps(f)
        def wrapper(*args, **kwargs):
            for _ in range(retry_count):
                # everything in here would be the same

        return wrapper
    return decorator

Then you can enable retrying for everyone, like so:

@retry(retry_count=5, delay=5)
def is_user_exist(username):
    try:
        pwd.getpwnam(username)
        log.info("User %s exist" % username)
        return True
    except KeyError:
        log.exception("User %s does not exist." % username)
        return False

Really fancy stuff

Why block when you're waiting? You could be doing so much more (this is for Python 3.5 and above) using asyncio. There isn't built-in support for this before that, but I know there are asynchronous frameworks that should be able to accomplish the same task.

By awaiting an asynchronous function that just runs for n seconds, you achieve the same goal but allow other asynchronous functions to do work while you're just waiting. Note that depending on the event loop you might end up waiting for slightly more or less time.

I also cleaned up the issues I mentioned about handling exceptions; it now always returns the result of the function if it has one, and it'll re-raise the exception without losing any traceback if there was one. that also uses a Python 3 only feature; I've left a comment for how ot do it in Python 2.

import functools
import asyncio    

def retry(retry_count=5, delay=5, allowed_exceptions=()):
    def decorator(f):
        @functools.wraps(f)
        def wrapper(*args, **kwargs):
            result = None
            last_exception = None
            for _ in range(retry_count):
                try:
                    result = func(*func_args, **kwargs)
                    if result: return result
                except Exception as e:
                    if e not in allowed_exceptions:
                        raise
                    last_exception = e
                log.debug("Waiting for %s seconds before retrying again")
                await asyncio.sleep(delay)

            if last_exception is not None:
                raise type(last_exception) from last_exception
                # Python 2
                # import sys
                # raise type(last_exception), type(last_exception)(last_exception), sys.exc_info()[2]

            return result

        return wrapper
    return decorator
Dan Oberlam
  • 8k
  • 2
  • 33
  • 74