CallExecutor fails if null is returned

[mkopylec]

It is quite common that retryable call will produce null result. Now it is impossible to return null because of:

    private Optional<T> tryCall(Callable<T> callable) throws UnexpectedException {
        try {
            T result = callable.call();
            return Optional.of(result); // TODO Allow nulls, use ofNullable(result);
        } catch (Exception e) {
            if (shouldThrowException(e)) {
                logger.trace("Throwing expected exception {}", e);
                throw new UnexpectedException(e);
            } else {
                lastKnownExceptionThatCausedRetry = e;
                return Optional.empty();
            }
        }
    }

[elennick]

So I feel like i may have designed myself into a bit of a corner here. Retry4j really only considers three result scenarios:

1. an expected exception is thrown and it retries (or fails because too many retries have occurred)
2. an unexpected exception is thrown and it fails
3. ANY result is returned and it succeeds

The problem that you're pointing out here is that when a null value comes back and there is logic expects that ANY value is a success executes, it blows up with a NPE due to that value being null.

So anyhow, I could do what you're suggesting above and use Optional.ofNullable() instead of Optional.of() but then I'm adding in another case above where:

4. a null result is returned and the executor retries (or fails because of too many retries)

...that seems potentially ok?

I might also just want to refactor the call executor to not use optionals the way it is now in which case a null result would just be considered a success which feels a little more natural and intuitive. Let me think about this a little before I rush a change.

[mkopylec]

What I expect is that the null result is a valid result and doesn't trigger retries.
As for the implementation just do it as you like. I don't know the code well so I will not suggest any solution :)

[elennick]

Yea... in retrospect i might have overcomplicated things when thinking about it, the entire issue is just caused by me using an optional internally. Already have it fixed, I'll push it out soon.

[elennick]

fixed with 0.7.3 release just now