Python code to retry function
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
17
down vote
favorite
I am trying to write a python function which retries a given function until given time or function returns True with given delay.
I have written the following function, but was thinking if there is a better way to do it?
Example function I want to retry for
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
My retry function
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
python python-2.7
 |Â
show 5 more comments
up vote
17
down vote
favorite
I am trying to write a python function which retries a given function until given time or function returns True with given delay.
I have written the following function, but was thinking if there is a better way to do it?
Example function I want to retry for
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
My retry function
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
python python-2.7
1
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers.
â Mast
Feb 28 at 18:01
2
@PatrickMevzek unless I misread that post, it doesn't seem possible to do with context managers
â Dannnno
Feb 28 at 18:09
1
In my experience, retry functions are a code smell to begin with.
â jpmc26
Mar 1 at 5:40
1
@Dannnno It's usually an indication you're doing asynch wrong, like calling sleep is. The OP's specific example is local: if the user isn't already there, why will it be there in the near future? In the best case, they've got some process running asynchronously that might create it. But the more reliable way to do this is to wait for an entry in a queue after the user is created. If you're talking about something low level like implementing TCP, you have a point. But most code today is written at a higher level, and failure at these levels usually indicates a retry isn't going to work.
â jpmc26
Mar 1 at 13:00
1
Very typically if you're doing retry you should probably use exponential/Fibonacci sleep times, for several reasons.
â Wayne Werner
Mar 6 at 3:47
 |Â
show 5 more comments
up vote
17
down vote
favorite
up vote
17
down vote
favorite
I am trying to write a python function which retries a given function until given time or function returns True with given delay.
I have written the following function, but was thinking if there is a better way to do it?
Example function I want to retry for
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
My retry function
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
python python-2.7
I am trying to write a python function which retries a given function until given time or function returns True with given delay.
I have written the following function, but was thinking if there is a better way to do it?
Example function I want to retry for
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
My retry function
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
python python-2.7
edited Feb 28 at 18:01
Mast
7,33663484
7,33663484
asked Feb 28 at 15:52
Gaurang Shah
2266
2266
1
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers.
â Mast
Feb 28 at 18:01
2
@PatrickMevzek unless I misread that post, it doesn't seem possible to do with context managers
â Dannnno
Feb 28 at 18:09
1
In my experience, retry functions are a code smell to begin with.
â jpmc26
Mar 1 at 5:40
1
@Dannnno It's usually an indication you're doing asynch wrong, like calling sleep is. The OP's specific example is local: if the user isn't already there, why will it be there in the near future? In the best case, they've got some process running asynchronously that might create it. But the more reliable way to do this is to wait for an entry in a queue after the user is created. If you're talking about something low level like implementing TCP, you have a point. But most code today is written at a higher level, and failure at these levels usually indicates a retry isn't going to work.
â jpmc26
Mar 1 at 13:00
1
Very typically if you're doing retry you should probably use exponential/Fibonacci sleep times, for several reasons.
â Wayne Werner
Mar 6 at 3:47
 |Â
show 5 more comments
1
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers.
â Mast
Feb 28 at 18:01
2
@PatrickMevzek unless I misread that post, it doesn't seem possible to do with context managers
â Dannnno
Feb 28 at 18:09
1
In my experience, retry functions are a code smell to begin with.
â jpmc26
Mar 1 at 5:40
1
@Dannnno It's usually an indication you're doing asynch wrong, like calling sleep is. The OP's specific example is local: if the user isn't already there, why will it be there in the near future? In the best case, they've got some process running asynchronously that might create it. But the more reliable way to do this is to wait for an entry in a queue after the user is created. If you're talking about something low level like implementing TCP, you have a point. But most code today is written at a higher level, and failure at these levels usually indicates a retry isn't going to work.
â jpmc26
Mar 1 at 13:00
1
Very typically if you're doing retry you should probably use exponential/Fibonacci sleep times, for several reasons.
â Wayne Werner
Mar 6 at 3:47
1
1
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers.
â Mast
Feb 28 at 18:01
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers.
â Mast
Feb 28 at 18:01
2
2
@PatrickMevzek unless I misread that post, it doesn't seem possible to do with context managers
â Dannnno
Feb 28 at 18:09
@PatrickMevzek unless I misread that post, it doesn't seem possible to do with context managers
â Dannnno
Feb 28 at 18:09
1
1
In my experience, retry functions are a code smell to begin with.
â jpmc26
Mar 1 at 5:40
In my experience, retry functions are a code smell to begin with.
â jpmc26
Mar 1 at 5:40
1
1
@Dannnno It's usually an indication you're doing asynch wrong, like calling sleep is. The OP's specific example is local: if the user isn't already there, why will it be there in the near future? In the best case, they've got some process running asynchronously that might create it. But the more reliable way to do this is to wait for an entry in a queue after the user is created. If you're talking about something low level like implementing TCP, you have a point. But most code today is written at a higher level, and failure at these levels usually indicates a retry isn't going to work.
â jpmc26
Mar 1 at 13:00
@Dannnno It's usually an indication you're doing asynch wrong, like calling sleep is. The OP's specific example is local: if the user isn't already there, why will it be there in the near future? In the best case, they've got some process running asynchronously that might create it. But the more reliable way to do this is to wait for an entry in a queue after the user is created. If you're talking about something low level like implementing TCP, you have a point. But most code today is written at a higher level, and failure at these levels usually indicates a retry isn't going to work.
â jpmc26
Mar 1 at 13:00
1
1
Very typically if you're doing retry you should probably use exponential/Fibonacci sleep times, for several reasons.
â Wayne Werner
Mar 6 at 3:47
Very typically if you're doing retry you should probably use exponential/Fibonacci sleep times, for several reasons.
â Wayne Werner
Mar 6 at 3:47
 |Â
show 5 more comments
5 Answers
5
active
oldest
votes
up vote
15
down vote
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, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
for _ in range(retry_count):
try:
result = func(*args, **kwargs)
if result: return result
except allowed_exceptions as e:
pass
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 await
ing 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 to do it in Python 2.
Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.
import functools
import asyncio
def retry(retry_count=5, delay=5, allowed_exceptions=()):
def decorator(f):
@functools.wraps(f)
async 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 allowed_exceptions as e:
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
add a comment |Â
up vote
12
down vote
The only thing I noticed is that retry
has a potentially inconsistent behavior.
Let me explain:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
If
func
is successful while checking it inside thewhile
,
None
will be returned. On the other hand, if it is successful
outside thewhile
, it will return whateverfunc
returns (in your exampleTrue
). You do not want to have that..
So I would propose a slight re-coding:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count): # all tries happen in the loop
if func(*func_args):
return True # if we succeed we return True
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return False # if we did not, we return False
You can get a bit fancier if you want to by subsituting the above for
loop with this:
for _ in range(retry_count):
res = func(*func_args) or log.debug("waiting for %s seconds before retyring again")
if res is None:
sleep(delay)
else:
return True
Note that I am assuiming here that log.debug
returns None
but it does not really matter as long as it does not return True
.
Strictly speaking, it might not returnTrue
outside of the loop; it'll return whateverfunc
does, which is probably something that can look truth-y or false-y.
â Dannnno
Feb 28 at 16:17
@Dannnno Correct, I am just referring to OP's examplefunc
.
â Ev. Kounis
Feb 28 at 16:19
You don't passkwargs
tofunc
, do you?
â Eric Duminil
Feb 28 at 21:50
add a comment |Â
up vote
2
down vote
Theory
Your retry
function is very similar to the structure of any
.
Keeping only the essential, you could write retry
as :
any(func(*func_args) for _ in range(count))
Code
If you want kwargs
, log
and sleep
, you can write:
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
Notes
log.debug
andtime.sleep
are both falsy, sofunc or log or time
is truthy if and only iffunc
is truthy.dict.pop
is needed to extractcount
anddelay
fromkwargs
. They would get passed tofunc
otherwise.
Complete code
import time
import pwd
import logging as log
def is_user(username):
try:
pwd.getpwnam(username)
log.info("User %s exist" % username)
return True
except KeyError:
log.error("User %s does not exist." % username)
return False
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
retry(is_user, 'username', count=10, delay=0.5)
1
I'm not a huge fan of usingany
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think thator
ing together everything is somewhat ugly - that should be a separate function imo.
â Dannnno
Feb 28 at 22:00
1
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing iffunc
will work at least once incount
times, and that's pretty much whatany
is for. All the rest is optional decoration IMHO.
â Eric Duminil
Feb 28 at 22:21
add a comment |Â
up vote
1
down vote
Here are some problems with your current setup:
- The function being retried can't take keyword arguments. This can be fixed pretty easily for the most part, but allowing the function to take arguments like
delay
will be more complicated. - To use retries, you have to call
retry(is_user_exist, username, ...)
. This makes it harder to avoid repetition. - You may end up with the same traceback appearing in the logs 5 times in a row.
retry
requires that the function returns things in a certain way. This will be annoying for some functions when you have to add extra lines, and terrible for other functions where a falsy return value is valid.
I suggest a decorator such as the one below. I wrote this decorator a while ago and have happily used it in several places. The idea is similar to Dannnno's answer, but I only retry after exceptions and don't pay attention to return values.
def retry(num_attempts=3, exception_class=Exception, log=None, sleeptime=1):
def decorator(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
for i in range(num_attempts):
try:
return func(*args, **kwargs)
except exception_class as e:
if i == num_attempts - 1:
raise
else:
if log:
log.error('Failed with error %r, trying again', e)
sleep(sleeptime)
return wrapper
return decorator
Here is some usage in real code:
from requests.exceptions import ConnectionError
@retry(5, ConnectionError, log)
def request(self, method, url='', **kwargs):
...
Here's another example where I only retry at the call site, rather than changing the definition of the function:
retry(5, Exception, log)(ftp.get)(path, destination)
Your case is a bit unusual because an exception is involved but you ultimately don't want to raise it. You could perhaps rewrite your code as follows:
if is_user_exist(username):
process_existing_user()
else:
process_nonexistent_user()
becomes:
try:
retry(5, KeyError, log)(pwd.getpwnam)(username)
except KeyError:
process_nonexistent_user()
else:
process_existing_user()
If you have other functions which you want to retry when a condition is false that don't involve exceptions, you could explicitly raise an exception:
class StuffNotFound:
pass
@retry(exception_class=StuffNotFound)
def process_stuff():
stuff = get_stuff():
if not stuff:
raise StuffNotFound()
process(stuff)
Ultimately the problem with this question is that we're talking about how to write a very generic and widely applicable function, but we only have one use case to apply it to. If you have other examples of code you want to retry, this discussion can be more informed.
add a comment |Â
up vote
-2
down vote
You can replace your while loops and retry_count
countdown with a simple for loop via range()
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count):
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return func(*func_args)
3
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
add a comment |Â
5 Answers
5
active
oldest
votes
5 Answers
5
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
15
down vote
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, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
for _ in range(retry_count):
try:
result = func(*args, **kwargs)
if result: return result
except allowed_exceptions as e:
pass
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 await
ing 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 to do it in Python 2.
Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.
import functools
import asyncio
def retry(retry_count=5, delay=5, allowed_exceptions=()):
def decorator(f):
@functools.wraps(f)
async 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 allowed_exceptions as e:
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
add a comment |Â
up vote
15
down vote
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, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
for _ in range(retry_count):
try:
result = func(*args, **kwargs)
if result: return result
except allowed_exceptions as e:
pass
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 await
ing 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 to do it in Python 2.
Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.
import functools
import asyncio
def retry(retry_count=5, delay=5, allowed_exceptions=()):
def decorator(f):
@functools.wraps(f)
async 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 allowed_exceptions as e:
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
add a comment |Â
up vote
15
down vote
up vote
15
down vote
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, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
for _ in range(retry_count):
try:
result = func(*args, **kwargs)
if result: return result
except allowed_exceptions as e:
pass
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 await
ing 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 to do it in Python 2.
Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.
import functools
import asyncio
def retry(retry_count=5, delay=5, allowed_exceptions=()):
def decorator(f):
@functools.wraps(f)
async 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 allowed_exceptions as e:
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
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, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
for _ in range(retry_count):
try:
result = func(*args, **kwargs)
if result: return result
except allowed_exceptions as e:
pass
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 await
ing 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 to do it in Python 2.
Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.
import functools
import asyncio
def retry(retry_count=5, delay=5, allowed_exceptions=()):
def decorator(f):
@functools.wraps(f)
async 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 allowed_exceptions as e:
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
edited Mar 5 at 15:03
answered Feb 28 at 16:45
Dannnno
5,3781649
5,3781649
add a comment |Â
add a comment |Â
up vote
12
down vote
The only thing I noticed is that retry
has a potentially inconsistent behavior.
Let me explain:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
If
func
is successful while checking it inside thewhile
,
None
will be returned. On the other hand, if it is successful
outside thewhile
, it will return whateverfunc
returns (in your exampleTrue
). You do not want to have that..
So I would propose a slight re-coding:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count): # all tries happen in the loop
if func(*func_args):
return True # if we succeed we return True
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return False # if we did not, we return False
You can get a bit fancier if you want to by subsituting the above for
loop with this:
for _ in range(retry_count):
res = func(*func_args) or log.debug("waiting for %s seconds before retyring again")
if res is None:
sleep(delay)
else:
return True
Note that I am assuiming here that log.debug
returns None
but it does not really matter as long as it does not return True
.
Strictly speaking, it might not returnTrue
outside of the loop; it'll return whateverfunc
does, which is probably something that can look truth-y or false-y.
â Dannnno
Feb 28 at 16:17
@Dannnno Correct, I am just referring to OP's examplefunc
.
â Ev. Kounis
Feb 28 at 16:19
You don't passkwargs
tofunc
, do you?
â Eric Duminil
Feb 28 at 21:50
add a comment |Â
up vote
12
down vote
The only thing I noticed is that retry
has a potentially inconsistent behavior.
Let me explain:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
If
func
is successful while checking it inside thewhile
,
None
will be returned. On the other hand, if it is successful
outside thewhile
, it will return whateverfunc
returns (in your exampleTrue
). You do not want to have that..
So I would propose a slight re-coding:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count): # all tries happen in the loop
if func(*func_args):
return True # if we succeed we return True
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return False # if we did not, we return False
You can get a bit fancier if you want to by subsituting the above for
loop with this:
for _ in range(retry_count):
res = func(*func_args) or log.debug("waiting for %s seconds before retyring again")
if res is None:
sleep(delay)
else:
return True
Note that I am assuiming here that log.debug
returns None
but it does not really matter as long as it does not return True
.
Strictly speaking, it might not returnTrue
outside of the loop; it'll return whateverfunc
does, which is probably something that can look truth-y or false-y.
â Dannnno
Feb 28 at 16:17
@Dannnno Correct, I am just referring to OP's examplefunc
.
â Ev. Kounis
Feb 28 at 16:19
You don't passkwargs
tofunc
, do you?
â Eric Duminil
Feb 28 at 21:50
add a comment |Â
up vote
12
down vote
up vote
12
down vote
The only thing I noticed is that retry
has a potentially inconsistent behavior.
Let me explain:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
If
func
is successful while checking it inside thewhile
,
None
will be returned. On the other hand, if it is successful
outside thewhile
, it will return whateverfunc
returns (in your exampleTrue
). You do not want to have that..
So I would propose a slight re-coding:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count): # all tries happen in the loop
if func(*func_args):
return True # if we succeed we return True
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return False # if we did not, we return False
You can get a bit fancier if you want to by subsituting the above for
loop with this:
for _ in range(retry_count):
res = func(*func_args) or log.debug("waiting for %s seconds before retyring again")
if res is None:
sleep(delay)
else:
return True
Note that I am assuiming here that log.debug
returns None
but it does not really matter as long as it does not return True
.
The only thing I noticed is that retry
has a potentially inconsistent behavior.
Let me explain:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
while retry_count > 1:
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
retry_count = retry_count - 1
return func(*func_args)
If
func
is successful while checking it inside thewhile
,
None
will be returned. On the other hand, if it is successful
outside thewhile
, it will return whateverfunc
returns (in your exampleTrue
). You do not want to have that..
So I would propose a slight re-coding:
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count): # all tries happen in the loop
if func(*func_args):
return True # if we succeed we return True
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return False # if we did not, we return False
You can get a bit fancier if you want to by subsituting the above for
loop with this:
for _ in range(retry_count):
res = func(*func_args) or log.debug("waiting for %s seconds before retyring again")
if res is None:
sleep(delay)
else:
return True
Note that I am assuiming here that log.debug
returns None
but it does not really matter as long as it does not return True
.
edited Feb 28 at 16:26
answered Feb 28 at 16:10
Ev. Kounis
620313
620313
Strictly speaking, it might not returnTrue
outside of the loop; it'll return whateverfunc
does, which is probably something that can look truth-y or false-y.
â Dannnno
Feb 28 at 16:17
@Dannnno Correct, I am just referring to OP's examplefunc
.
â Ev. Kounis
Feb 28 at 16:19
You don't passkwargs
tofunc
, do you?
â Eric Duminil
Feb 28 at 21:50
add a comment |Â
Strictly speaking, it might not returnTrue
outside of the loop; it'll return whateverfunc
does, which is probably something that can look truth-y or false-y.
â Dannnno
Feb 28 at 16:17
@Dannnno Correct, I am just referring to OP's examplefunc
.
â Ev. Kounis
Feb 28 at 16:19
You don't passkwargs
tofunc
, do you?
â Eric Duminil
Feb 28 at 21:50
Strictly speaking, it might not return
True
outside of the loop; it'll return whatever func
does, which is probably something that can look truth-y or false-y.â Dannnno
Feb 28 at 16:17
Strictly speaking, it might not return
True
outside of the loop; it'll return whatever func
does, which is probably something that can look truth-y or false-y.â Dannnno
Feb 28 at 16:17
@Dannnno Correct, I am just referring to OP's example
func
.â Ev. Kounis
Feb 28 at 16:19
@Dannnno Correct, I am just referring to OP's example
func
.â Ev. Kounis
Feb 28 at 16:19
You don't pass
kwargs
to func
, do you?â Eric Duminil
Feb 28 at 21:50
You don't pass
kwargs
to func
, do you?â Eric Duminil
Feb 28 at 21:50
add a comment |Â
up vote
2
down vote
Theory
Your retry
function is very similar to the structure of any
.
Keeping only the essential, you could write retry
as :
any(func(*func_args) for _ in range(count))
Code
If you want kwargs
, log
and sleep
, you can write:
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
Notes
log.debug
andtime.sleep
are both falsy, sofunc or log or time
is truthy if and only iffunc
is truthy.dict.pop
is needed to extractcount
anddelay
fromkwargs
. They would get passed tofunc
otherwise.
Complete code
import time
import pwd
import logging as log
def is_user(username):
try:
pwd.getpwnam(username)
log.info("User %s exist" % username)
return True
except KeyError:
log.error("User %s does not exist." % username)
return False
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
retry(is_user, 'username', count=10, delay=0.5)
1
I'm not a huge fan of usingany
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think thator
ing together everything is somewhat ugly - that should be a separate function imo.
â Dannnno
Feb 28 at 22:00
1
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing iffunc
will work at least once incount
times, and that's pretty much whatany
is for. All the rest is optional decoration IMHO.
â Eric Duminil
Feb 28 at 22:21
add a comment |Â
up vote
2
down vote
Theory
Your retry
function is very similar to the structure of any
.
Keeping only the essential, you could write retry
as :
any(func(*func_args) for _ in range(count))
Code
If you want kwargs
, log
and sleep
, you can write:
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
Notes
log.debug
andtime.sleep
are both falsy, sofunc or log or time
is truthy if and only iffunc
is truthy.dict.pop
is needed to extractcount
anddelay
fromkwargs
. They would get passed tofunc
otherwise.
Complete code
import time
import pwd
import logging as log
def is_user(username):
try:
pwd.getpwnam(username)
log.info("User %s exist" % username)
return True
except KeyError:
log.error("User %s does not exist." % username)
return False
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
retry(is_user, 'username', count=10, delay=0.5)
1
I'm not a huge fan of usingany
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think thator
ing together everything is somewhat ugly - that should be a separate function imo.
â Dannnno
Feb 28 at 22:00
1
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing iffunc
will work at least once incount
times, and that's pretty much whatany
is for. All the rest is optional decoration IMHO.
â Eric Duminil
Feb 28 at 22:21
add a comment |Â
up vote
2
down vote
up vote
2
down vote
Theory
Your retry
function is very similar to the structure of any
.
Keeping only the essential, you could write retry
as :
any(func(*func_args) for _ in range(count))
Code
If you want kwargs
, log
and sleep
, you can write:
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
Notes
log.debug
andtime.sleep
are both falsy, sofunc or log or time
is truthy if and only iffunc
is truthy.dict.pop
is needed to extractcount
anddelay
fromkwargs
. They would get passed tofunc
otherwise.
Complete code
import time
import pwd
import logging as log
def is_user(username):
try:
pwd.getpwnam(username)
log.info("User %s exist" % username)
return True
except KeyError:
log.error("User %s does not exist." % username)
return False
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
retry(is_user, 'username', count=10, delay=0.5)
Theory
Your retry
function is very similar to the structure of any
.
Keeping only the essential, you could write retry
as :
any(func(*func_args) for _ in range(count))
Code
If you want kwargs
, log
and sleep
, you can write:
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
Notes
log.debug
andtime.sleep
are both falsy, sofunc or log or time
is truthy if and only iffunc
is truthy.dict.pop
is needed to extractcount
anddelay
fromkwargs
. They would get passed tofunc
otherwise.
Complete code
import time
import pwd
import logging as log
def is_user(username):
try:
pwd.getpwnam(username)
log.info("User %s exist" % username)
return True
except KeyError:
log.error("User %s does not exist." % username)
return False
def retry(func, *func_args, **kwargs):
count = kwargs.pop("count", 5)
delay = kwargs.pop("delay", 5)
return any(func(*func_args, **kwargs)
or log.debug("waiting for %s seconds before retyring again" % delay)
or time.sleep(delay)
for _ in range(count))
retry(is_user, 'username', count=10, delay=0.5)
edited Feb 28 at 21:49
answered Feb 28 at 21:40
Eric Duminil
1,8501613
1,8501613
1
I'm not a huge fan of usingany
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think thator
ing together everything is somewhat ugly - that should be a separate function imo.
â Dannnno
Feb 28 at 22:00
1
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing iffunc
will work at least once incount
times, and that's pretty much whatany
is for. All the rest is optional decoration IMHO.
â Eric Duminil
Feb 28 at 22:21
add a comment |Â
1
I'm not a huge fan of usingany
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think thator
ing together everything is somewhat ugly - that should be a separate function imo.
â Dannnno
Feb 28 at 22:00
1
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing iffunc
will work at least once incount
times, and that's pretty much whatany
is for. All the rest is optional decoration IMHO.
â Eric Duminil
Feb 28 at 22:21
1
1
I'm not a huge fan of using
any
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think that or
ing together everything is somewhat ugly - that should be a separate function imo.â Dannnno
Feb 28 at 22:00
I'm not a huge fan of using
any
here; if you have a very limited set of functionality you want then it works fine and is very simple, but understanding why you're doing it that way is somewhat unintuitive. I also think that or
ing together everything is somewhat ugly - that should be a separate function imo.â Dannnno
Feb 28 at 22:00
1
1
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing if
func
will work at least once in count
times, and that's pretty much what any
is for. All the rest is optional decoration IMHO.â Eric Duminil
Feb 28 at 22:21
@Dannnno: Thanks for the comment. To each his own, I guess. OP's is basically interested in knowing if
func
will work at least once in count
times, and that's pretty much what any
is for. All the rest is optional decoration IMHO.â Eric Duminil
Feb 28 at 22:21
add a comment |Â
up vote
1
down vote
Here are some problems with your current setup:
- The function being retried can't take keyword arguments. This can be fixed pretty easily for the most part, but allowing the function to take arguments like
delay
will be more complicated. - To use retries, you have to call
retry(is_user_exist, username, ...)
. This makes it harder to avoid repetition. - You may end up with the same traceback appearing in the logs 5 times in a row.
retry
requires that the function returns things in a certain way. This will be annoying for some functions when you have to add extra lines, and terrible for other functions where a falsy return value is valid.
I suggest a decorator such as the one below. I wrote this decorator a while ago and have happily used it in several places. The idea is similar to Dannnno's answer, but I only retry after exceptions and don't pay attention to return values.
def retry(num_attempts=3, exception_class=Exception, log=None, sleeptime=1):
def decorator(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
for i in range(num_attempts):
try:
return func(*args, **kwargs)
except exception_class as e:
if i == num_attempts - 1:
raise
else:
if log:
log.error('Failed with error %r, trying again', e)
sleep(sleeptime)
return wrapper
return decorator
Here is some usage in real code:
from requests.exceptions import ConnectionError
@retry(5, ConnectionError, log)
def request(self, method, url='', **kwargs):
...
Here's another example where I only retry at the call site, rather than changing the definition of the function:
retry(5, Exception, log)(ftp.get)(path, destination)
Your case is a bit unusual because an exception is involved but you ultimately don't want to raise it. You could perhaps rewrite your code as follows:
if is_user_exist(username):
process_existing_user()
else:
process_nonexistent_user()
becomes:
try:
retry(5, KeyError, log)(pwd.getpwnam)(username)
except KeyError:
process_nonexistent_user()
else:
process_existing_user()
If you have other functions which you want to retry when a condition is false that don't involve exceptions, you could explicitly raise an exception:
class StuffNotFound:
pass
@retry(exception_class=StuffNotFound)
def process_stuff():
stuff = get_stuff():
if not stuff:
raise StuffNotFound()
process(stuff)
Ultimately the problem with this question is that we're talking about how to write a very generic and widely applicable function, but we only have one use case to apply it to. If you have other examples of code you want to retry, this discussion can be more informed.
add a comment |Â
up vote
1
down vote
Here are some problems with your current setup:
- The function being retried can't take keyword arguments. This can be fixed pretty easily for the most part, but allowing the function to take arguments like
delay
will be more complicated. - To use retries, you have to call
retry(is_user_exist, username, ...)
. This makes it harder to avoid repetition. - You may end up with the same traceback appearing in the logs 5 times in a row.
retry
requires that the function returns things in a certain way. This will be annoying for some functions when you have to add extra lines, and terrible for other functions where a falsy return value is valid.
I suggest a decorator such as the one below. I wrote this decorator a while ago and have happily used it in several places. The idea is similar to Dannnno's answer, but I only retry after exceptions and don't pay attention to return values.
def retry(num_attempts=3, exception_class=Exception, log=None, sleeptime=1):
def decorator(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
for i in range(num_attempts):
try:
return func(*args, **kwargs)
except exception_class as e:
if i == num_attempts - 1:
raise
else:
if log:
log.error('Failed with error %r, trying again', e)
sleep(sleeptime)
return wrapper
return decorator
Here is some usage in real code:
from requests.exceptions import ConnectionError
@retry(5, ConnectionError, log)
def request(self, method, url='', **kwargs):
...
Here's another example where I only retry at the call site, rather than changing the definition of the function:
retry(5, Exception, log)(ftp.get)(path, destination)
Your case is a bit unusual because an exception is involved but you ultimately don't want to raise it. You could perhaps rewrite your code as follows:
if is_user_exist(username):
process_existing_user()
else:
process_nonexistent_user()
becomes:
try:
retry(5, KeyError, log)(pwd.getpwnam)(username)
except KeyError:
process_nonexistent_user()
else:
process_existing_user()
If you have other functions which you want to retry when a condition is false that don't involve exceptions, you could explicitly raise an exception:
class StuffNotFound:
pass
@retry(exception_class=StuffNotFound)
def process_stuff():
stuff = get_stuff():
if not stuff:
raise StuffNotFound()
process(stuff)
Ultimately the problem with this question is that we're talking about how to write a very generic and widely applicable function, but we only have one use case to apply it to. If you have other examples of code you want to retry, this discussion can be more informed.
add a comment |Â
up vote
1
down vote
up vote
1
down vote
Here are some problems with your current setup:
- The function being retried can't take keyword arguments. This can be fixed pretty easily for the most part, but allowing the function to take arguments like
delay
will be more complicated. - To use retries, you have to call
retry(is_user_exist, username, ...)
. This makes it harder to avoid repetition. - You may end up with the same traceback appearing in the logs 5 times in a row.
retry
requires that the function returns things in a certain way. This will be annoying for some functions when you have to add extra lines, and terrible for other functions where a falsy return value is valid.
I suggest a decorator such as the one below. I wrote this decorator a while ago and have happily used it in several places. The idea is similar to Dannnno's answer, but I only retry after exceptions and don't pay attention to return values.
def retry(num_attempts=3, exception_class=Exception, log=None, sleeptime=1):
def decorator(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
for i in range(num_attempts):
try:
return func(*args, **kwargs)
except exception_class as e:
if i == num_attempts - 1:
raise
else:
if log:
log.error('Failed with error %r, trying again', e)
sleep(sleeptime)
return wrapper
return decorator
Here is some usage in real code:
from requests.exceptions import ConnectionError
@retry(5, ConnectionError, log)
def request(self, method, url='', **kwargs):
...
Here's another example where I only retry at the call site, rather than changing the definition of the function:
retry(5, Exception, log)(ftp.get)(path, destination)
Your case is a bit unusual because an exception is involved but you ultimately don't want to raise it. You could perhaps rewrite your code as follows:
if is_user_exist(username):
process_existing_user()
else:
process_nonexistent_user()
becomes:
try:
retry(5, KeyError, log)(pwd.getpwnam)(username)
except KeyError:
process_nonexistent_user()
else:
process_existing_user()
If you have other functions which you want to retry when a condition is false that don't involve exceptions, you could explicitly raise an exception:
class StuffNotFound:
pass
@retry(exception_class=StuffNotFound)
def process_stuff():
stuff = get_stuff():
if not stuff:
raise StuffNotFound()
process(stuff)
Ultimately the problem with this question is that we're talking about how to write a very generic and widely applicable function, but we only have one use case to apply it to. If you have other examples of code you want to retry, this discussion can be more informed.
Here are some problems with your current setup:
- The function being retried can't take keyword arguments. This can be fixed pretty easily for the most part, but allowing the function to take arguments like
delay
will be more complicated. - To use retries, you have to call
retry(is_user_exist, username, ...)
. This makes it harder to avoid repetition. - You may end up with the same traceback appearing in the logs 5 times in a row.
retry
requires that the function returns things in a certain way. This will be annoying for some functions when you have to add extra lines, and terrible for other functions where a falsy return value is valid.
I suggest a decorator such as the one below. I wrote this decorator a while ago and have happily used it in several places. The idea is similar to Dannnno's answer, but I only retry after exceptions and don't pay attention to return values.
def retry(num_attempts=3, exception_class=Exception, log=None, sleeptime=1):
def decorator(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
for i in range(num_attempts):
try:
return func(*args, **kwargs)
except exception_class as e:
if i == num_attempts - 1:
raise
else:
if log:
log.error('Failed with error %r, trying again', e)
sleep(sleeptime)
return wrapper
return decorator
Here is some usage in real code:
from requests.exceptions import ConnectionError
@retry(5, ConnectionError, log)
def request(self, method, url='', **kwargs):
...
Here's another example where I only retry at the call site, rather than changing the definition of the function:
retry(5, Exception, log)(ftp.get)(path, destination)
Your case is a bit unusual because an exception is involved but you ultimately don't want to raise it. You could perhaps rewrite your code as follows:
if is_user_exist(username):
process_existing_user()
else:
process_nonexistent_user()
becomes:
try:
retry(5, KeyError, log)(pwd.getpwnam)(username)
except KeyError:
process_nonexistent_user()
else:
process_existing_user()
If you have other functions which you want to retry when a condition is false that don't involve exceptions, you could explicitly raise an exception:
class StuffNotFound:
pass
@retry(exception_class=StuffNotFound)
def process_stuff():
stuff = get_stuff():
if not stuff:
raise StuffNotFound()
process(stuff)
Ultimately the problem with this question is that we're talking about how to write a very generic and widely applicable function, but we only have one use case to apply it to. If you have other examples of code you want to retry, this discussion can be more informed.
answered Mar 1 at 13:23
Alex Hall
33113
33113
add a comment |Â
add a comment |Â
up vote
-2
down vote
You can replace your while loops and retry_count
countdown with a simple for loop via range()
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count):
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return func(*func_args)
3
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
add a comment |Â
up vote
-2
down vote
You can replace your while loops and retry_count
countdown with a simple for loop via range()
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count):
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return func(*func_args)
3
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
add a comment |Â
up vote
-2
down vote
up vote
-2
down vote
You can replace your while loops and retry_count
countdown with a simple for loop via range()
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count):
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return func(*func_args)
You can replace your while loops and retry_count
countdown with a simple for loop via range()
def retry(func, *func_args, **kwargs):
retry_count = kwargs.get("retry_count", 5)
delay = kwargs.get("delay", 5)
for _ in range(retry_count):
if func(*func_args):
return
log.debug("waiting for %s seconds before retyring again")
sleep(delay)
return func(*func_args)
answered Feb 28 at 17:59
user171782
1
1
3
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
add a comment |Â
3
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
3
3
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
The point your making was already made in another, earlier answer. I suggest to either edit this answer so that it adds new value, or else delete it.
â janos
Feb 28 at 18:57
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
@janos I saw that after posting it but I cannot figure out how to delete this post on the mobile app. I will when I get on the desktop site
â user171782
Feb 28 at 20:09
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f188539%2fpython-code-to-retry-function%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
1
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers.
â Mast
Feb 28 at 18:01
2
@PatrickMevzek unless I misread that post, it doesn't seem possible to do with context managers
â Dannnno
Feb 28 at 18:09
1
In my experience, retry functions are a code smell to begin with.
â jpmc26
Mar 1 at 5:40
1
@Dannnno It's usually an indication you're doing asynch wrong, like calling sleep is. The OP's specific example is local: if the user isn't already there, why will it be there in the near future? In the best case, they've got some process running asynchronously that might create it. But the more reliable way to do this is to wait for an entry in a queue after the user is created. If you're talking about something low level like implementing TCP, you have a point. But most code today is written at a higher level, and failure at these levels usually indicates a retry isn't going to work.
â jpmc26
Mar 1 at 13:00
1
Very typically if you're doing retry you should probably use exponential/Fibonacci sleep times, for several reasons.
â Wayne Werner
Mar 6 at 3:47