call cleanup hook only if we decide to have next attempt

[kcwu]

What I mean is, change original code from

            if cleanup:
                cleanup()
            if n == attempts:
                log.info("retry: Giving up on %s", action_name)
                raise

to

            if n == attempts:
                log.info("retry: Giving up on %s", action_name)
                raise
            if cleanup:
                cleanup()

Two benefits of this change:

1. The cleanup may be costly. Doing cleanup without more attempts is a waste.
2. If the last attempt failed, it is better to keep the environment as-is for developer investigation. Cleanup may be destructive and make debugging harder.

For original behavior (do something after exception), it could be done inside action() using normal try..except.

How do you think? If you think it is good change, I can send PR.

[bhearsum]

Some cleanup involves doing something before the next attempt to ensure the next attempts succeeds. I can also conceive of a case where cleanup needs to be done after the last attempt, to reset some state (for whatever might be running next), or possibly even remove some secrets that you don't want hanging around. I'm not really sure what the more common case is, so it's tough to say which we should optimize for.

In any case, retrier can be used to work around this:

for i, _ in enumerate(retrier(attempts=5), start=1):
    try:
        # code you want to retry goes here
    except Something as e:
        if i < 5:
            # cleanup goes here
        logging.exception("Hit error...")

[kcwu]

It's okay for retrier. But unable to work around for users of retry, retriable, and retrying with current api.

[bhearsum]

Yeah, that's true. retrier is the basis for the other functions though, so you should be able to workaround?

If we move the change when the cleanup happens in retrier, we may break existing use cases, so I'm hesitant to do it without some further thought.

[kcwu]

Right, incompatibility is scary.

Other ideas:

• additional callback function.
• like Pass exceptions to the cleanup method #40 , passing more information to cleanup . For example, passing either current iteration count, n, or a boolean variable indicating whether more iterations or not.

[bhearsum]

Right, incompatibility is scary.

Incompatibility is OK, I just think we should change this without a compelling reason.

like #40 , passing more information to cleanup . For example, passing either current iteration count, n, or a boolean variable indicating whether more iterations or not.

This is an interesting idea. It feels like a very appropriate thing to be passed to the cleanup function. I'd accept a PR for this.

It would be nice to use some introspection to not cause a breaking change. This can be done by using inspect.getargspec (Python 2.7), inspect.getfullargspec (3.0 - 3.3), or inspect.signature (3.4+) to see what the callback is expecting to receive. Thanks to @catlee for this idea.