RFC: Reworking the Exceptions of connect() and login()

Smack Smack Dev
21

As an active user of SMACK, I’d like to add my 2¢ here:

@CSH: I like the idea of reusing existing generic exceptions, however the specific LoginExceptions from javax.security.auth.login are not available on Android, making my use-case more complicated to implement. Therefore, I would rather suggest having our own subclasses of LoginException.

I want to be able to abort automatic reconnects if the condition is not going to go away, i.e. if the user entered wrong credentials. I can see how this is a grey area with temporarily disabled logins, but I want to have all generic cases covered. I can imagine that IOException for everything connection-related, including timeouts, is the sane way to go here.

I know for sure that “network unreachable” and “unknown host” exceptions happen when the Android device is not connected to the Internet. The latter is reasonable to assume as permanent on a Desktop PC though.

@Daniele Ricci: I am not sure we can/should introduce StanzaExceptions, as the errors in parsing individual stanzas happen in the parser thread, not in the main thread when you call blocking functions. Therefore, the blocking functions like connect()/login() should never throw them anyway.

I am against adding an isTemporary() method to our own exceptions. It should become clear from the exception class what kind of exception it is, provided it can become clear at all.

@Flow: I really do not like SMACK wrapping everything into XMPPException, it would be much better if XMPPException would only cover stream errors, while connection errors are passed up as-is.

22

In times of async. IO one could drop the exceptions completely.

So one could register a callback handler or query the state of the conn./login and access the raw xmpp error msg if needed.

For devs it is more friendly if they don’t have to work it out on their own whether an error is a temporary or permanent problem.

23

LG wrote:

In times of async. IO one could drop the exceptions completely.

So one could register a callback handler or query the state of the conn./login and access the raw xmpp error msg if needed.

For devs it is more friendly if they don’t have to work it out on their own whether an error is a temporary or permanent problem.

Devs are the only ones who can determine whether an exception is temporary or permanent based on the context in which their application is being used. We cannot make that decision for them, it will inevitably be wrong.

24

rcollier wrote:

Devs are the only ones who can determine whether an exception is temporary or permanent based on the context in which their application is being used. We cannot make that decision for them, it will inevitably be wrong.

I believe it is possible to make sane default decissions on what is a temporary error and what not. Of course this decisions will not be correct in every use-case, that’s why it’s imporant not to enforce them. But I would guess that 90% (or so) of the Smack users will be happy with the default categorization and will profit because the library has taken another burden from them. And that’s what a good library should do: Offload as much as generic code as possible from the user, and to come with sane defaults.

And the others, who need XY not to be/to be an temporary/permanent exception can easily overwrite the defaults or ignore them alltogether.

25

Hello guys,
Any news about this discussion on checked vs unchecked exceptions?
I think it’s awful to be forced to code like this:

try {
    connection.login();
    //..................
} catch (XMPPException|SmackException|InterruptedException|IOException e) {
    //Exception handling: show that the login has failed
}

It doesn’t make sense to handle those exception separately because they are too generic. What could I say to the user when an InterruptedException or SmackException happens? For any of those, I just say that it was not possible to login. This makes smake utilization too hard.

Maybe unchecked exception containing the specific cause (such as the ones in javax.security.auth.login) is better. That will make it less painful to use the API.
For generic exceptions that may happen, Java even has classes such as UncheckedIOException. SmackException could extend RuntimeException instead of Exception, making it unchecked.

The same issue happens for XMPPTCPConnection.connect().

26

I think it does. XMPPConnection is thrown on errors on the XMPP protocol layer, SmackException is thrown in case of internal Smack errors, The other two are standard.

Although I don’t think that this is a good practice, Java’s try/multi-catch makes it easy to do so.

Unchecked exceptions for a network library are a permanent cause of surprise. Hence I decided against them

Correct me if I am wrong, but it appears that at least some exceptions in javax.security.auth.login are checked exceptions, e.g. AccountExpiredException (Java Platform SE 7 )

27

I think it does. XMPPException is thrown on errors on the XMPP protocol layer, SmackException is thrown in case of internal Smack errors, The other two are standard.

I understand they are different exceptions. However, if any of those exceptions happen, there is no way to recover from the error. It means the user could not connect or login, so the client application cant proceed. The recommendation is to use unchecked exceptions.

In any of those cases, the only error message we could show to the user is either “It was not possible to connect: (host not found, timeout, etc)” or “It was not possible to login: (incorrect username/password, inactive account, etc)”.

Although I don’t think that this is a good practice, Java’s try/multi-catch makes it easy to do so.

Multi-catch is a bad practice when you have the option to handle each exception in a different way but you are doing the same thing for all of them instead. If there is an error in the XMPP protocol layer or an internal Smack error during connection or login, the client app simply cannot proceed.

Unchecked exceptions for a network library are a permanent cause of surprise. Hence I decided against them

You can use JavaDoc to document any exception a method may throw, if it’s checked or not. This way, there will be no surprise. The developer will have to handle them somewhere. He/she has the option to handle individually or in a multi-catch. The point in using unchecked exceptions is that you give freedom for the developer and make things easier.

Correct me if I am wrong, but it appears that at least some exceptions in javax.security.auth.login are checked exceptions

Oh, yeah. But anyway, the method could throw some specific exception as those, but preferably, unchecked ones. However, if those Java.security exceptions are used, at least they are very specific.