Standardise on when exceptions are thrown and when callbacks are used

[mattheworiordan]

See https://github.com/ably/ably-java/blob/master/lib/src/main/java/io/ably/lib/realtime/Channel.java#L494-L497

The publish method can raise an AblyException, but also has a completionListener which handles success & failure conditions.

Now if you look at https://goo.gl/Hc0yqK, you can see that if a channel is in invalid state we raise an exception, yet if RTL6c3 was implemented correctly it should fail it the channel becomes FAILED / SUSPENDED before the message is published.

IMHO this is wrong and confusing for developers. See ably/ably-ruby@92686ae where I have changed this in Ruby, and https://github.com/ably/wiki/issues/175 (points 6 & 7).

I believe our policy for async methods / methods with callbacks should be that if the arguments provided are invalid then raise an exception, otherwise any exceptions relating to the state of the library or future state of the library should be passed as a failure argument in the callback

┆Issue is synchronized with this Jira Task by Unito

[paddybyers]

This is becoming a bit of an issue now. We have different behaviours in different situations, and tests that expect those different behaviours, and we need to take some steps towards consistency even if we don't change everything wholesale.

For example, an enter() that fails because the library clientId is * throws an exception, whereas an enterClient() that fails with an incompatible clientId calls the error callback. I think this is broken. Further, an enter() that fails because the token clientId is * fails asynchronously if the library was still connecting at the time of the call; and other enter() failures (eg being in the failed state) fail with an exception.

I think we need to decide on what the API should really be and, without changing all existing behaviours, at least ensure that we are moving as consistently as possible towards that when we make changes, as we are now for 0.9.

Until now we've tended to implement async operations in separate steps:

• check the arguments against the state of the library, and throw exceptions on any error;
• initiate the requested async operation;
• report the result of the async operation via the listener.

For the realtime library, for methods that take a CompletionListener or some other asynchronous listener, the default behaviour should be for the callback to be called, even when the error can be determined by the library at the time of the call. I think clear programming errors (eg null pointer exceptions) should continue to be exceptions, but errors that the application programmer cannot prevent (because they depend on his inputs, such as credentials, or on the state of the network or the service) should come consistently via the listener callback for APIs that have callbacks.

The things that are affected by this are primarily channel state operations and publish/presence operations. Everything else in the realtime library (auth, history, stats, request, etc) inherit from the rest library (ie are not reimplemented in the realtime library) and are blocking calls that throw exceptions.

Just so we know the magnitude of the issue I'm going to look at making the clientId issues mentioned above consistent for presence, and we'll see how much would need to change as a result.

[mattheworiordan]

Well yes, I couldn't agree more, hence the issue and ably/docs#252.

I think the logic should be quite simple:

• Sync methods always throw exceptions
• Async methods / methods will result callbacks only throw exceptions when the inputs for the method are invalid irrespective of the transient state of the library, otherwise the callback is called with an error.

[paddybyers]

I've updated the clientId checks for presence and messages: 2076a29

Channel state problems still throw exceptions, but I think this covers the new functionality.

[mattheworiordan]

New instance of what a customer considers to be a "crash".

We have not manually closed the library, but we're seeing crashes with this trace:

io.ably.lib.types.AblyException$HostFailedException: java.lang.Exception: Connection unavailable
       at io.ably.lib.realtime.ChannelBase.publish(ChannelBase.java:781)
       at io.ably.lib.realtime.ChannelBase.publish(ChannelBase.java:765)
       at io.ably.lib.realtime.ChannelBase.publish(ChannelBase.java:730)

@paddybyers is this something @amihaiemil could perhaps look at as I assume we need consistency in this library with other libraries?

[paddybyers]

So the question is whether or not we're happy to make this incompatible change to the behaviour, and have these state conditions generate errors in the listener instead of an exception. If we're going to do that, is it a "benign" change, in that a client will have to be handing either kind of error in any case, or do we wait until we're making an explicit compatibility break in the API?

[mattheworiordan]

Well in Ruby, I follow the policy that:

• 40x-like errors that are invalid because the inputs are incorrect, and can be determined by the library regardless of the state of the underlying connection, are raised as exceptions there and then. I believe this is right because this is a developer mistake i.e. they are calling methods with invalid arguments.
• In the sync lib, calling any method that fails, raises an exception.
• In the async lib, calling any method or operation that fails, will only ever result in an error callback, or some instance an error being emitted on the object if not directly related.

I don't think it's really ever valid to raise an exception that could crash an app that is due to the non-deterministic state of a library.

I don't think we should wait to make this change. This is just wrong now IMHO, and customers have to listen for callback / event emitted failures anyway.

[amihaiemil]

@mattheworiordan @paddybyers

I'm open to have a look. It just seems to me you're discussing a much broader topic and am not sure where to start. If you can give me more particular info, it would be great. So far I understand you would like ChannelBase.publish to call the fail Listener rather than throw an exception when the Connection is down?

[amihaiemil]

@paddybyers any details here? :D