JedisCluster throws NullPointerException when maxAttempts is set to 0

[shivincible]

When maxAttempts is set to 0 in JedisCluster, it leads to an unhandled NullPointerException during command execution. The cause is that lastException remains null, and Throwable.addSuppressed(null) is called, which is not allowed.

This could be avoided with a guard clause to validate the value of maxAttempts or by checking for null before calling addSuppressed.

Reproduction

JedisCluster jedis = new JedisCluster(
    new HostAndPort("localhost", 6379),
    2000,       // connection timeout
    2000,       // soTimeout
    0,          // maxAttempts
    null,       // password
    new JedisPoolConfig()
);
jedis.set("key", "value");  // NPE thrown

Stacktrace
java.lang.NullPointerException: Cannot suppress a null exception. at java.util.Objects.requireNonNull(Objects.java:235) at java.lang.Throwable.addSuppressed(Throwable.java:1074) at redis.clients.jedis.JedisClusterCommand.runWithRetries(JedisClusterCommand.java:158)

Cause
In RetryableCommandExecutor.java, the code attempts to call lastException.addSuppressed(ex) without checking whether lastException is null.

e.addSuppressed(lastException); // lastException is null when maxAttempts = 0

This was tried with Jedis 3.9.0, but the same issue is also present in the latest master branch.
Relevant line in the current code:
https://github.com/redis/jedis/blob/master/src/main/java/redis/clients/jedis/executors/RetryableCommandExecutor.java#L71

[shivincible]

Fix for this: #4187

[ggivo]

@shivincible
Thanks for reporting it!
Looking at the code, maxAttempt=0 does not make sense since this way we will also skip the command execution itself.
We should also add some parameter validation. Will open a PR shortly.

[shivincible]

Hi @ggivo — we ran into a NullPointerException when setting maxAttempts = 0, which can be a valid use case (especially during controlled experiments, like the ones we were running at Uber). fyi @atakavci

I've submitted a fix to safely handle this scenario and avoid the NPE:
👉 #4185 (comment)

Would really appreciate it if you could take a look when you get a chance. Thanks!

[ggivo]

Hi @shivincible
I saw your initial gist suggested in #4185 (comment) , and extended it a bit with parameter validation and covering also ClusterCommandExecutor

You are mentioning that there is a valid use case to set the maxAttempts = 0, which will effectively disable the command execution, and will immediately throw an exception JedisException("No more attempts left.").

Could you elaborate on your use case?

[atakavci]

hi @shivincible , thank you for the comment and PR you submit, we appriciate it a lot.
i see the use case, though.. i am not %100 sure this is the best approach.
the way i see it;
if you want to do experiments with a controlled environment, i think a client library that's configured to malfunction should not be a part of the client library itself.
more over,, to configure a more realistic setup, i think you should either mock it or setup with some underlying services or components that are not in place at all or failing by intention.
i ll leave this up to @ggivo and you to discuss.

[shivincible]

Thanks @ggivo and @atakavci — really appreciate you both taking the time to review and weigh in.

Our use case at Uber was focused on simulating controlled failure scenarios, where disabling retries entirely (maxAttempts = 0) helped validate fallback behavior further up the stack. We weren’t aiming for this to be part of production config—just wanted clean failure without relying on mocks or external setup.

That said, I saw the updated PR @ggivo put up (thanks for extending the logic!)—looks like it covers the validation piece more comprehensively, which is great. My original intent was mainly to avoid the NPE and make this behavior explicit for edge cases like ours.

Personally, I still feel it makes sense to allow maxAttempts = 0 as a valid config for cases where fail-fast behavior is intentional and controlled. But totally open to where the team wants to draw that line—happy to align either way.

Let me know if there's anything I can help clarify or adjust on my end. Appreciate the collaboration!

[ggivo]

@shivincible
Thanks for taking the time to report it and also provide a fix.

I think for testing, mocking erroneous behaviour is the way to go.
Hope you don't mind adding you as a co-author to the final PR proposal.

[shivincible]

Thanks @ggivo — appreciate you taking this forward. Totally makes sense that mocking would be the preferred approach for simulating failures during testing. In our case, we found the maxAttempts = 0 config to be a quick way to simulate that behavior without needing extra setup, but I get that it's not the typical pattern.

And sure, feel free to add me as a co-author—thanks for the heads-up!