RaftSyncCounter.set() swallows transient leader unavailability as RuntimeException via CompletableFutures.join()

[mzazaipsc]

Hi,

In JGroups-RAFT, many of the synchronous methods internally call their async counterparts in a blocking way. This is fine in general — for example, if the leader or quorum isn’t available, a TimeoutException can be thrown, and it’s appropriate for the application to handle that.

Problem

One place where this becomes problematic is in RaftSyncCounter.set(). This method internally calls asyncSet(), but blocks using CompletableFutures.join(), which wraps any underlying exceptions into a RuntimeException without exposing the root cause directly.

This differs from other methods in JGroups-RAFT that are allowed to declare throws Exception and thus let callers handle expected distributed failures (like leader unavailability).

The issue is that RaftSyncCounter implements the SyncCounter interface, and SyncCounter.set() does not declare any checked exceptions. As a result:

• RaftSyncCounter.set() cannot throw a TimeoutException or similar checked exceptions

• All failures get wrapped in a RuntimeException, making transient failures look catastrophic

• Applications can’t easily distinguish between a temporary leader election and a genuine bug

This is especially concerning because leader elections or temporary unavailability are normal in RAFT, and shouldn’t cause the app to crash or misinterpret the failure.

I’m not sure if the use of join() here was intentional under the assumption that updating counter should always succeed while the cluster is up (in JGroups context) — but in the RAFT context, temporary unavailability is expected and shouldn’t be treated as fatal.

Solution

Possible solutions could include:

• Stop implementing SyncCounter in RaftSyncCounter, allowing it to declare "throws Exception" like other methods in JGroups-RAFT.
• Or modifying SyncCounter to declare checked exceptions on set() (but this is outside the scope of JGroups-RAFT).

Thanks for considering this issue. I’d be interested in your thoughts on whether this behavior was intentional, and what direction you’d prefer for a fix. I’m happy to implement the change if a solution is agreed upon.

[jabolina]

Thanks again for reporting, @mipsc!

Extending org.jgroups.blocks.atomic.SyncCounter was deliberate for future integrations, so we should keep that. Unfortunately, changing the definition in JGroups would require at least a minor release. An alternative I can think of is:

• Keep all existing methods for RaftSyncCounter;
• Expand RaftSyncCounter with a checked counterpart for every other method. void set(long value -> void setChecked(long value) throws Exception;
• Implement checked methods without CompletableFutures.join method and use the .get() method for checked exceptions. I believe these would still be wrapped by a CompletionException or similar.

What do you think? This would keep the same interfaces, avoid changing JGroups, and wouldn't break existing code.

[mzazaipsc]

Hi @jabolina,

Thanks for the quick and thoughtful response!

I totally understand the need to stick with SyncCounter for consistency and future extensibility. While I think adding *checked() counterparts is a pragmatic and a well-balanced solution, but how I’d approach this really depends on whether changing SyncCounter in JGroups is something that could be considered in a future (minor) release.

If that is an option, then I’d be hesitant to introduce new methods like setChecked() — they feel like a workaround for a deeper issue, and I’m concerned they could clutter the API or become legacy once SyncCounter evolves (and would then be difficult to remove). In this case, I’d suggest updating the JavaDoc for set() and other affected methods with a warning to clarify the current behavior and recommend asyncSet().get() as a workaround for better failure handling in the meantime.

However, if modifying SyncCounter might not be considered if it would break consistency or require broader changes across JGroups, Infinispan, etc., then adding a *Checked() version may indeed be the most practical path forward.

If evolving SyncCounter is something you’d be open to, I’d be happy to help draft a proposal or contribute a PR.

Let me know your thoughts — and thanks again for the great discussion!

[belaban]

Hi @mipsc
I'm in favor of changing SyncCounter in JGroups if this is the best way forward. If we decide to change it, let's make sure to get it right and possibly apply this kind of change to other methods too. We should also ask folks on the JGroups mailing list if someone uses SyncCounter. Perhaps @pruivo can comment on this, too?

[pruivo]

No offense, but setChecked(long) throws Exception sounds like a very crap API.

I agree that the checked exception is missing from the sync counter interface, but let's do it properly instead of a generic Exception.

It should probably be something like throws InterruptedException, TimeoutException, CounterException. Document that CounterException is the exception to be used by implementations when they have implementation-specific errors.

I wouldn't recommend doing this change in a patch release and waiting for a minor or major release.

[mzazaipsc]

Hi @pruivo, thanks for the thoughtful response!

but let's do it properly instead of a generic Exception.

Couldn't agree more. But this IS unfortunately very common across jgroups-raft (e.g. ReplicatedStateMachine) already (and partially in JGroups too). I guessed this is because of the fact that internally most sync methods call the async counterpart too. And as a result, it looked to me that this:

It should probably be something like throws InterruptedException..

Has been avoided deliberately to not expose the exceptions thrown by CompletableFuture.get() to the caller, shadowing the async call under the hood and reducing complexity on the caller side. So that declaring that the method throws Exception and it has to be dealt with gracefully is reliable enough. What by SyncCounter is exactly needed too. So throws Exception would keep some kind of consistency across the sync raft methods in terms of exception handling.

I like the idea of CounterException and more specific exceptions in general, but I honestly suggest it might come in a broader scope requiring kinda figuring out a more consistent exceptions model across different interfaces/methods throwing generic Exception? A point I wanted to arise in a different, less critical issue.

So the benefit here by throwing the generic exception is achieving the expected reliability, by forcing the user to deal with the method as it CAN at anytime, because of a transient failure eventually, throw an exception, and that that exception must be handled, instead of it crashing the app. While throwing more specific exceptions might be part of a broader enhancement, that would bring better exception handling across the parts of the both libraries with similar semantics (throws Exception)?

Same thing maybe with CompletableFutures.join(), where there might be some scanning needed for user-facing methods, where it is internally used, and eventually gets replaced there with .get(), so that, again, it forces exception handling!

[cfredri4]

Not related to this change, but I would also appreciate more specific exceptions in JGroups. Maybe a candidate for JGroups 6? 🙂
(and as well as less checked exceptions, as that's the direction Java is taking)

[pruivo]

Has been avoided deliberately to not expose the exceptions thrown by CompletableFuture.get() to the caller, shadowing the async call under the hood and reducing complexity on the caller side.

It is not shadowing anything, it is the standard in Java. If the thread is waiting for something, you need to be aware of interrupt requests and don't want to wait until you're retired to get an answer. Everybody knows what it does by looking at the method signature with explicit exception. IMO, set(long) or set(long) throws Exception is the same thing.

[mzazaipsc]

It is not shadowing anything, it is the standard in Java.

throws Exception in the library is actually not exposing the checked exceptions thrown by CompletableFuture.get underneath. That is what I mean by it is, where it is used, shadowing the async call, as it seems. I don’t, however, know the reason it’s made like this. My guess was that it is deliberate to hide the async underlying call.

IMO, set(long) or set(long) throws Exception is the same thing.

set(long) will eventually throw RuntimeException swallowing all checked exceptions, because of CompletableFuture.join(). Causing no compiler error if no exception handling was made by the caller -> causing the app to fail during transient failures (leader unavailable) which is VERY common in Raft -> serious reliability issue. So they are far from being the same thing. Checked exceptions ARE NOT the same as unchecked exceptions.

The issue here is not how the API looks like (which is important, and is actually a more generic issue across the library: throws Exception is everywhere). It is a reliability issue in the RAFT context. And throws Exception, while not informative enough, solves that reliability issue.

[belaban]

Not related to this change, but I would also appreciate more specific exceptions in JGroups. Maybe a candidate for JGroups 6? 🙂 (and as well as less checked exceptions, as that's the direction Java is taking)

Not sure I understand: do you want more checked or unchecked exceptions? I assume the former...
I support replacing throws Exception with throws CounterException, FileNotFoundException etc, as this doesn't break compatibility.
However, as Pedro said, adding checked exceptions in a superclass or interface needs to be done in a new minor version.
As I said, do you haver users of SyncCounter?

[cfredri4]

Both. 🙂
I.e. more specific exceptions (for clarity) and more unchecked exceptions (because that's where Java is heading).

This is a good summary:

Falling Out Of Favour
There is a good reason why Java is one of the only widely used programming languages that has checked exceptions (refer to this StackOverflow question). Other JVM languages like Kotlin, Scala, Groovy, Clojure etc. do not have checked exceptions - at least, they don't force you to handle them. The Kotlin language documentation summarises the reasoning quite well, and cites the 2003 interview with lead language designer of C# on the rationale for skipping checked exceptions.

Even within the Java language itself, you only need to look at newer APIs in the standard library to realise that checked exceptions should be used sparingly. APIs introduced in Java 8 such as java.time, java.util.stream and java.util.function all avoid checked exceptions. The now well-established Streams API is actively hostile to checked exceptions. This has caused a lot of pain for developers who want to use the streams API because it requires hacky workarounds to propagate checked exceptions from within Java's built-in functional interfaces. More on this later.

Edit: just to clarify, I'm talking in general and not about SyncCounter specifically (we don't use SyncCounter).

[belaban]

Do you have any specific examples of what you'd like to see changed? E.g. in JChannel.
I've had a few requests for adding checked exceptions, and others for removing them, in the last 27 years. I'm not a big fans of checked exceptions myself, but then again this is a matter of taste...

[belaban]

Re SyncCounter: I'm not religious about jgroups-raft having to implement it. I'd be totally ok with you guys creating your own interface in jgroups-raft...
I also don't think this is widely used, so not too important whatever you guys decide to do.

[pruivo]

Both. 🙂 I.e. more specific exceptions (for clarity) and more unchecked exceptions (because that's where Java is heading).

This is a good summary:

Falling Out Of Favour
There is a good reason why Java is one of the only widely used programming languages that has checked exceptions (refer to this StackOverflow question). Other JVM languages like Kotlin, Scala, Groovy, Clojure etc. do not have checked exceptions - at least, they don't force you to handle them. The Kotlin language documentation summarises the reasoning quite well, and cites the 2003 interview with lead language designer of C# on the rationale for skipping checked exceptions.
Even within the Java language itself, you only need to look at newer APIs in the standard library to realise that checked exceptions should be used sparingly. APIs introduced in Java 8 such as java.time, java.util.stream and java.util.function all avoid checked exceptions. The now well-established Streams API is actively hostile to checked exceptions. This has caused a lot of pain for developers who want to use the streams API because it requires hacky workarounds to propagate checked exceptions from within Java's built-in functional interfaces. More on this later.

Edit: just to clarify, I'm talking in general and not about SyncCounter specifically (we don't use SyncCounter).

@cfredri4 thanks for sharing this, I've just watched the YouTube video. I have mixed feelings about it, to be honest. I'll keep this in mind for the future APIs.

@belaban (sarcasm mode on), you can remove SyncCounter and force users to use AsynCounter. They can use CompletableFuture.join() when they want unchecked exceptions or CompletableFuture.get() for checked exceptions (sarcasm mode off)

[mzazaipsc]

Both. 🙂 I.e. more specific exceptions (for clarity) and more unchecked exceptions (because that's where Java is heading).

This is a good summary:

Falling Out Of Favour
There is a good reason why Java is one of the only widely used programming languages that has checked exceptions (refer to this StackOverflow question). Other JVM languages like Kotlin, Scala, Groovy, Clojure etc. do not have checked exceptions - at least, they don't force you to handle them. The Kotlin language documentation summarises the reasoning quite well, and cites the 2003 interview with lead language designer of C# on the rationale for skipping checked exceptions.
Even within the Java language itself, you only need to look at newer APIs in the standard library to realise that checked exceptions should be used sparingly. APIs introduced in Java 8 such as java.time, java.util.stream and java.util.function all avoid checked exceptions. The now well-established Streams API is actively hostile to checked exceptions. This has caused a lot of pain for developers who want to use the streams API because it requires hacky workarounds to propagate checked exceptions from within Java's built-in functional interfaces. More on this later.

Edit: just to clarify, I'm talking in general and not about SyncCounter specifically (we don't use SyncCounter).

Hi @cfredri4, nice article. I think checked exceptions are great if the application can recover from the error (excellent example here where a request is submitted during election etc.) Unchecked are for cases where the application either cannot recover, or that they could have been avoided by validation (e.g. index out of range), IMO.

Regarding exceptions and this comment by @belaban: belaban/JGroups#900 (comment), I would suggest:

first adding throws Exception to the interface (just like ReplicatedStateMachine's methods) for minor (or major) release then refine later with more specific exceptions in a broader context (for SyncCounter and others) in a next version. This would make the possibility of the transient failure explicit to the API caller at least, enabling reliable usage of it, on par with other raft methods. Adding specific exceptions later, would not break compatibility, as Bela mentioned before.

Whatever is agreed upon, I would be happy to change the PR based on it.

This would be very practical too:

Re SyncCounter: I'm not religious about jgroups-raft having to implement it. I'd be totally ok with you guys creating your own interface in jgroups-raft... I also don't think this is widely used, so not too important whatever you guys decide to do.