It is planned to rework the exceptions thrown by connect() and login() with the next major Smack release. While this may take a while I am going to ask the community here how these Exceptions should look like and which ones are currently missing.
The current proposal of exceptions can be seen in SMACK-426 description.
IMHO there is two factors that need to be distinguished:
Is it an temproary or permantent exception/error
Is it an XMPP error, something defined in a XMPP spec or is it something else (e.g. could not connect to port 1234)
Now, we have different approaches to design this: Either as subclasses, as methods or as a combination of both.
A possible design could look like this
---- PermanentXmppExcepton
-------- PermantentProtocolException
-------- PermantentSmackException
----- TemproraryXmppException
-------- TemporaryProtocolException
-------- TemprorarySmackException
I am currently working on my own XMPP library and therefore put some thoughts into exception handling as well:
For the connect() method I just throw an IOException, which could also be an java.net.UnknownHostException, e.g. if the connection to a port failed. Usually during stream negotiation, there is no stream error, so I thought this is sufficient. Not sure, if this is good, but in the worst case you could still pass a cause to IOException.
For the login() method I throw a javax.security.auth.login.LoginException.
For more specific errors during SASL negotation, I throw sub-exceptions like javax.security.auth.login.AccountLockedException () or javax.security.auth.login.CredentialExpiredException () or just
javax.security.auth.login.FailedLoginException ()
Every other SASL failure either should not happen (e.g. ) or one could make own subclasses of LoginException, if really needed.
Other exceptions during login, which can happen during resource binding (stanza errors, e.g. ) are also handled via LoginException currently (text only). But I am not sure, if this is optimal.
Then there’s also compression (XEP-138) in between SASL and resource binding (XEP-170), which might produce failures. If that happens, I just ignore it (XEP-138: “the initiating entity is free to retry the compression negotiation if it fails.”)
And what do you mean by a temporary or permanent exception?
CSH wrote:
And what do you mean by a temporary or permanent exception?
If you are writing a piece of software that uses either Babbler or Smack, you will find yourself in the situation where you want to handle exceptions in the connect/login stage. And the feedback we always get, and that is also my opinion, is that it’s not really important what the exceptions caused (of course you should give feedback about the cause, maybe even to the user) but it’s important if it’s a temporary failure reason, which means that a connection/login retry should be retried, or a permanent one.
try {
connection.connect();
connection.login();
} catch (XmppException e) {
if (e.isTemporary()) {
// schedule reconnect
} else {
// abort
}
}
Ah I understand. But it’s hard to tell.
E.g. consider:
The server is not reachable. This could either be, that the server does not exist, or it is temporarily not available (e.g. due to bad connection or maintainence), or the connection from the client is temporarily not available (e.g. bad WLAN).
If the server or port just doesn’t exist, then it’s not worth retrying. If only your WLAN connection is bad, it’s worth retrying.
Or in login:
If the username/pw was wrong, it’s not worth retrying automatically (since user action is needed). On the other hand, if resource binding returned a it might be worth retrying, because the resource might be free the next time.
So on the one hand I think it’s worth to distinguish, on the other hand it’s probably hard to decide what is temporary and what is not (and opinions might even differ here).
CSH wrote:
So on the one hand I think it’s worth to distinguish, on the other hand it’s probably hard to decide what is temporary and what is not (and opinions might even differ here).
Right, but you just mentioned two prime examples for permanent errors:
Hi, I think that defining an exception as temporary is a prerogative of the developer. I mean of course an exception can be temporary or permanent, but I think it’s more of a semantic issue. I decide (based on what the exception is) whether an error is permanent or temporary. This is the kind of control a software library should have: do not suggest logic, rather provide tools to implement logic and let the developer decide. All of this IMHO.
Anyway, here are my contributions:
Stream errors can be easily mapped to exceptions and thrown when needed. Stanza errors are a bit different: you might want them to be thrown or listen to them via listeners. Maybe I would do something like a method to enable/disable stanza exception per-connection. Or even better, a way to “catch” stanza errors via listeners and, inside those listeners, decide wether to stop processing it or let it be thrown.
Yes, there might be some obvious ones, like
= permanent
= temporary
But there also some non-obvious ones, like . Maybe an admin or even the user itself can re-enable the account somehow. It’s of course also a matter of “how long do I want to retry and how often”.
And what about resource binding?
http://xmpp.org/rfcs/rfc6120.html#bind-clientsubmit-retries
suggests that a user might change it’s resource, if an error occured.
So, is every resource-binding error a permanent one, because the user has to do something?
I want to say: Yes, it’s probably useful to know if I should try to reconnect/relogin, but I am not sure, if it should be in Smack’s responsibility to decide that (because of different opinions).
Instead a developer could also do this:
if (e instanceof UnknownHostException)
{
// do not retry, permanent
}
if (e instanceof SocketTimeoutException)
{
// do retry
}
That should settle:
if it should be in Smack’s responsibility to decide that (because of different opinions).
Hi, I think that defining an exception as temporary is a prerogative of the developer. I mean of course an exception can be temporary or permanent, but I think it’s more of a semantic issue. I decide (based on what the exception is) whether an error is permanent or temporary. This is the kind of control a software library should have: do not suggest logic, rather provide tools to implement logic and let the developer decide. All of this IMHO.
I am with you. Exactly my thoughts 
- normal IOExceptions for socket and network-related errors (but I guess it’s already implemented like that. It would be just re-throwing though)
Again. My thoughts.
- two subclasses of XMPPException: StanzaException or StreamException, representing stanza errors and stream errors
- many subclasses as possible representing the stanza/stream error types or a method to set/get the error name (constants needed in this case)
Exactly my thoughts. I am getting nervous.
Not sure about your “catch exceptions via listeners”-idea, though.
Daniele Ricci wrote:
Hi, I think that defining an exception as temporary is a prerogative of the developer. I mean of course an exception can be temporary or permanent, but I think it’s more of a semantic issue. I decide (based on what the exception is) whether an error is permanent or temporary. This is the kind of control a software library should have: do not suggest logic, rather provide tools to implement logic and let the developer decide. All of this IMHO.
If Smack would, no matter how it’s implemented (methods/subclass), categorize exceptions in temporary or not, then it doesn’t mean that the user has to follow that decision. It’s just a common problem that every Smack user needs to solve, and providing the user with a good and sane default is exaclty what a library should do. This means shifting work from the user to the library. But we won’t enforce anything, if the user decides to handle an exception differently, he/she is free to do so.
Could you elaborate Stanza and Stream errors? Maybe with some examples? Does it, and if so, how, compares to Smack’s XMPPError?
Oh, and I’ve heard no argument until now, why it’s cool to use the intial exception (IOException, …) instead of wrapping it into XmppException. Would be happy to hear some.
I had your thoughts because I was writing while you posted another 2 replies Don’t get offended
Not sure about your “catch exceptions via listeners”-idea, though.
I mean that stanza errors can happen also during e.g. a simple iq request. We don’t want any iq error getting fired as a global exception. So we could either:
Guys this editor is awful. I can’t split quoting parts if I don’t modify the HTML code directly.
Flow wrote:
If Smack would, no matter how it’s implemented (methods/subclass), categorize exceptions in temporary or not, then it doesn’t mean that the user has to follow that decission. It’s just a common problem that every Smack user needs to solve, and providing the user with a good and sane default is exaclty what a library should do. This means shifting work from the user to the library. But we won’t enforce anything, if the user decides to handle an exception differently, he/she is free to do so.
Could you elaborate Stanza and Stream errors? Maybe with some examples? Does it, and if so, how, compares to Smack’s XMPPError?
Oh, and I’ve heard no argument until now, why it’s cool to use the intial exception (IOException, …) instead of wrapping it into XmppException. Would be happy to hear some.
Reply to par. 1: of course something like a method isTemporary would be useful. I find the class type Permanent/TemporaryException an enforcement. Anyway I would not use it
Reply to par. 2: does XMPPError cover stream errors too? If so, I’ll take back what I said, sorry.
Reply to par. 3: because IMHO IOExceptions and network related errors are not proper “XMPPExceptions”(though this might conflict with CSH statement about using java login exception classes). And also because you can do catch(IOException) instead of an all-catch catch(XMPPException)
Oh, and I’ve heard no argument until now, why it’s cool to use the intial exception (IOException, …) instead of wrapping it into XmppException. Would be happy to hear some.
Maybe read:
Item 60: Favor the use of standard exceptions
from “Effective Java”.
http://uet.vnu.edu.vn/~chauttm/e-books/java/Effective.Java.2nd.Edition.May.2008. 3000th.Release.pdf
Also the whole chapter 9 about exceptions is quite interesting and might be useful for this discussion.
Great link, sure a good read.
Item 61 describes the design pattern Smack uses: Throw exceptions appropriate to the abstraction (e.g. by wrapping them into XmppException).
I still see no convincing reason to use the initial exceptions instead of wrapping them into XmppExceptions. And I still think that it’s useful if Smack categorizes exceptions as temporary (at least).
Great link, sure a good read.
Definitively. The whole book is a very good read. If you haven’t read it, I recommend to do so. It has a lot of “do’s” and “don’ts” regarding API design and Java in general. At least I learned a lot.
Yet even some famous libraries (e.g. Apache HTTP client) use this paradigm:
http://hc.apache.org/httpcomponents-core-ga/httpcore/apidocs/org/apache/http/Htt pException.html
I don’t know if the library uses that class to wrap IOExceptions too, though.
Regarding temporary/permanent: a method (e.g. isTemporary) can be useful, but marking the difference so hard using two different subclasses is an exaggeration.
Although opionons will always vary, from my own experience and research, the recommended practice with respect to exception handling can be pretty much summed up with two points.
For example, any exception that basically represents a coding or logic error should be unchecked, as they should not occur. Things like IllegalArgumentException and IllegalStateException are common examples of these.
I don’t see the point of trying to distinguish exceptions as being temporary or permanent (I have never seen exceptions described in this manner, typically the only permanent exceptions are defined as Errors, are thrown by the VM, and are unrecoverable). How an exception is handled is always up to the developer, so the impact of any exception will be decided by the developer in the context of what their application does. The best approach is to simply document what exception can be thrown (both checked and unchecked) and for what reasons. The developer can decide what to do from there.
I think XMPPException should be used exclusively for errors in the stream itself (this includes errors returned in stanzas).
**EDIT: **My first point above does not preclude the usage of custom exceptions, but they should add some value. If the only exception that can occur in a method is an IOException, then that is what should be thrown. On the other hand, if that exception is a hidden implementation detail, then it should not be thrown, but wrapped into something that represents an error in the abstraction level that called it. The exception thrown on connect, for example has to take into account that there are multiple types of connections (Socket, BOSH and certainly at some point Websocket), and that each will have it’s own specific protocol specific exceptions internally.
Always favour unchecked exceptions, unless the caller has some reasonable expectation of being able to handle the exception.
From what I’ve learned:
checked exceptions for logical application-level errors.
unchecked exception for programmatical errors, i.e. if the developer made a mistake, like passing null to a method.
By the way, does that mean the next Smack release is going to break existing API?
I am pretty sure that Flow intends API changes for the next major release, whether the next one is 4.0 or 3.4.2 is something I don’t know.