Open mattheworiordan opened 7 years ago
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:
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.
Well yes, I couldn't agree more, hence the issue and https://github.com/ably/docs/issues/252.
I think the logic should be quite simple:
I've updated the clientId
checks for presence and messages: https://github.com/ably/ably-java/pull/294/commits/2076a2913febe23994695f06fc3dfbdf72acdef8
Channel state problems still throw exceptions, but I think this covers the new functionality.
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?
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?
Well in Ruby, I follow the policy that:
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.
@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?
@paddybyers any details here? :D
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 acompletionListener
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 https://github.com/ably/ably-ruby/commit/92686ae8a3666dba0727f6719f5505de2a6bed7e 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