add cause to exception constructor

[PhilipDukhov]

Let's say I have an exception in my network call during SubscriptionTokenGetter.getSubscriptionToken. I pass it to cb.Done, but when SubscriptionEventListener.onError is called, all I see in the stack trace is

I/System.out: io.github.centrifugal.centrifuge.SubscriptionTokenError
I/System.out:     at io.github.centrifugal.centrifuge.Subscription.lambda$sendSubscribe$4$io-github-centrifugal-centrifuge-Subscription(Subscription.java:272)
I/System.out:     at io.github.centrifugal.centrifuge.Subscription$$ExternalSyntheticLambda6.run(Unknown Source:6)
I/System.out:     at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:463)
...

It says nothing about the reason why my call failed. I could've check if it's SubscriptionTokenError and get throwable, but this would require me to know about all the exceptions SDK could throw.

That's why when you have an exception, and your create an other one with your details of how it happened, you should fill cause with the original exception.

Here's how stacktrace looks like in this case:

I/System.out: io.github.centrifugal.centrifuge.SubscriptionTokenError
I/System.out:     at io.github.centrifugal.centrifuge.Subscription.lambda$sendSubscribe$4$io-github-centrifugal-centrifuge-Subscription(Subscription.java:273)
I/System.out:     at io.github.centrifugal.centrifuge.Subscription$$ExternalSyntheticLambda6.run(Unknown Source:6)
I/System.out:     at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:463)
...

I/System.out: Caused by: java.lang.IllegalStateException: test exception message
I/System.out:     at com.test.centrifuge.CentrifugeManager$subscribeTo$1$subscription$1$1$getSubscriptionToken$1.invokeSuspend(CentrifugeManager.kt:139)
...

I guess you could remove throwable property from such exceptions, as it's available in cause - but it's up to you, as I guess it may change API.

Also, the difference between throwable and exception is that the former indicates that the current state of the application is faulty and cannot be recovered, so it must crash. For this reason nobody catches them - and catches Exception instead - it is better to get a crash in analytics to fix it than to ignore such an exception and get a malfunctioning application.

As your exceptions are not that critical, I would advise you to replace their superclass with exception - more for stability, since you're not really throwing them, so it shouldn't be a runtime problem unless someone would like to throw it at their own level to catch later.

[FZambia]

I actually thought about this but did not know how to quickly implement - thanks!

Regarding Throwable/Exception – these are good thoughts, do I understand right that you mean replacing this:

public class ConfigurationError extends Throwable {
    private final Throwable error;

    ConfigurationError(Throwable error) {
        this.error = error;
    }

    public Throwable getError() {
        return error;
    }
}

With this:

public class ConfigurationError extends Exception {
    ConfigurationError(Exception error) {
        this.initCause(error);
    }
}

?

[PhilipDukhov]

Yep, that's right.

[PhilipDukhov]

You can simply call super - it'll use the needed exception constructor

public class ConfigurationError extends Exception {
    ConfigurationError(Exception error) {
        super(error);
    }
}

[FZambia]

I see... though some things already use Throwable inside this SDK - it's WebSocket onFailure callback, also concurrent future exception callback. I tried to migrate to Exceptions but was not able to get rid of them everywhere in a clean way. Probably it's possible but I suppose Throwable is not that bad and the simplest thing would be stick to Throwable everywhere..

I created #58 – could you take a look? Do you think it's better approach than setting cause? I think it also solves issue you mentioned here. If starting from scratch (or maybe a bit later in future releases) I'd remove error property since it's accessible by getCause() method.

[FZambia]

Closing in favour of #58