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

Smack Smack Dev
8

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

}

9

That should settle:

if it should be in Smack’s responsibility to decide that (because of different opinions).

:slight_smile:

10

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 :wink:

  • 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.

11

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.

12

I had your thoughts because I was writing while you posted another 2 replies :wink: 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:

  • not consider stanza errors at all. Developer will read the stanza attributes manually and act accordingly
  • enable/disable stanza error throwing via a per-connection setting
  • let Smack throw a stanza exception if it was not previously listened to (e.g. unexpected/unlistened stanza error - this might be complicated though)
13

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)

14

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.

15

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).

16

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.

17

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.

18

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.

  1. Use the standard Java exceptions wherever applicable
  2. Always favour unchecked exceptions, unless the caller has some reasonable expectation of being able to handle the exception.

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.

19

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?

20

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.

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.