chore: Allow combining non base-derived objects for retries#485
chore: Allow combining non base-derived objects for retries#485
Conversation
tenacity/asyncio/retry.py
Outdated
| result = False | ||
| for r in self.retries: | ||
| result = result or await _utils.wrap_to_async_func(r)(retry_state) | ||
| result = result or (await _utils.wrap_to_async_func(r)(retry_state) is True) |
There was a problem hiding this comment.
I see you added is True everywhere. This seems like a different change not required. Are we sure we need this?
There was a problem hiding this comment.
While adding the tests I noticed that, if a wrong combination of strategies is added (combination of async strategies in a sync context), then we'd end up trying to and/or a coroutine, which hangs forever as it's never going to be resolved in the sync context. By adding the is True we directly check if the result is exactly that and avoid endlessly waiting for the coroutine.
There was a problem hiding this comment.
Actually, thinking about it a bit more... Maybe we'd rather let it hang? Not sure what's best they, without the changes it hangs forever, which is not very intuitive, and with these changes it finishes without failures but it does not run the strategy at all, giving an invalid outcome 😕
There was a problem hiding this comment.
Is it feasible to trap this invalid call pattern and raise an exception to make it obvious that there's a programming error?
There was a problem hiding this comment.
I came up with this, it is more coupled to the actual implementations, but it now fails on invalid scenarios so it's more explicit. I used a similar check to the one already existing in __init__.py, let me know what you think @jd f8725d6
3 participants
Fixes #481
Versions prior to introducing #451 inadvertently allowed to use plain callables as retry strategies, as opposed to
retry_basederiving classes, as they conform to the__call__typing. It is not a big must, but slightly changing the__and__/__or__checks would allow for that again.NOTE: combining non-async strategies with async functions will not work, other combinations should, as the tests show.