Capture context rich error messages

[sacOO7]

trafficstars

• Current ErrorInfo class only captures the exception message instead of the whole error context. e.g. https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java#L226
• Exception should be fully wrapped while being consumed.
• It shouldn't leave any missing trail of actual error source.
• Best exception handling practices - https://stackify.com/best-practices-exceptions-java/
• Link to the slack convo - https://ably-real-time.slack.com/archives/C019LDWM9UH/p1602442482002500
• Related to #606

┆Issue is synchronized with this Jira Task by Unito

[sacOO7]

Proposed solution

• Extend ErroInfo with native Exception class e.g. public class ErrorInfo extends Exception

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/types/ErrorInfo.java#L14 (as stated, ErrorInfo is an exception type encapsulating error information containing an Ably-specific error code and generic status code, but it's not a java exception ... yet)

• Pass original exception as a cause while creating new ErrorInfo object.

@Override
        public void onError(final Exception e) {    
            connectListener.onTransportUnavailable(WebSocketTransport.this, new ErrorInfo(e.getMessage(), 503, 80000, e));
       }

• Include original exception cause while logging ErrorInfo object.

[mattheworiordan]

I support this proposal, this is what I have done in Ruby, see https://github.com/ably/ably-ruby/blob/main/lib/ably/models/error_info.rb#L33-L57 and https://github.com/ably/ably-ruby/blob/main/lib/ably/exceptions.rb#L16.

@paddybyers @QuintinWillison WDYT?

In fact, shouldn't this be part of the spec, that where possible, we use a base exception class so that the underlying base exception itself is available.

[QuintinWillison]

We already have an AblyException class which encapsulates an ErrorInfo instance, which is an architectural choice - using composition - that had already been made (rightly or wrongly) in the past.

Modifying the inheritance tree above ErrorInfo (essentially widening its public interface) feels like a hammer to crack a nut, and is not the right place in the stack to do this.

There is already a well-known, idiomatic constructor on the base Exception class allowing us to pass a cause. Therefore this requirement may be solved by simply adding a constructor to our AblyException class that calls that.

Also, @sacOO7, can you please provide a breadcrumb link back to the original discussion that prompted you to create this issue so those passing by in future can gain more context as to the requirement. Thanks.

[sacOO7]

Yes, I think we already have AblyException class. I was looking into the use cases where we use standalone ErrorInfo class and AblyException derived from ErrorInfo class. It seems at some of the places, we are not using AblyException class effectively (Like current case). I was also looking into the way exceptions are being handled.

• Generally, it's not recommended to catch throwables i.e. https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/realtime/ChannelBase.java#L279 and https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/realtime/ChannelBase.java#L289

Throwable is the superclass of all exceptions and errors. In a catch clause, it will not only catch all exceptions; it will also catch all errors. Errors are thrown by the JVM to indicate serious problems that are not intended to be handled by an application. Typical examples for that are the OutOfMemoryError or the StackOverflowError. Both are caused by situations that are outside of the control of the application and can’t be handled.

I am still digging more to find where and how AblyException and ErrorInfo classes are being used. As @QuintinWillison suggested, there is a strong possibility that we don't have to introduce a new base class for ErrorInfo. Current implementation should suffice for handling errors if we use it properly.

[sacOO7]

So, I came up with findings after doing a deep analysis of current error handling implementation Observations are based on the following guidelines

1. Custom error handler shouldn't miss trail of actual error source

If you lose the stacktrace and message of the original exception, it will make it difficult to analyze the exceptional event that caused your exception. It becomes difficult while debugging the exact source of the issue The exception stacktrace is as important as error message

2. Code should not contain empty catch blocks.

Error caught is not clear it's not logged with a proper exception. You don’t know how the code will change in the future. Someone might remove the validation that prevented the exceptional event without recognizing that this creates a problem. Or the code that throws the exception gets changed and now throws multiple exceptions of the same class, and the calling code doesn’t prevent all of them.

3. Every exception should be logged using logger instead of System.err or println

When the logger is used for logging exception, it's easy to parse and read. Also, it's custom logger gives control over where to log the exception

4. Be specific about throwing and catching the exception

The more specific the exception that you throw is, the better. Always keep in mind that a coworker who doesn’t know your code (or maybe you in a few months) may need to call your method and handle the exception. And as a result, the caller of your method will be able to handle the exception better or avoid it with an additional check.

5. Throwables shouldn't be caught

Throwable is the superclass of all exceptions and errors. In a catch clause, it will not only catch all exceptions; it will also catch all errors. Errors are thrown by the JVM to indicate serious problems that are not intended to be handled by an application. Typical examples for that are the OutOfMemoryError or the StackOverflowError. Both are caused by situations that are outside of the control of the application and can’t be handled.

It seems we have code snippets that violate the above guidelines. Exceptions are supposed to be caught and well handled in order to get a clear idea about working/ non-working application behavior.

References -

1. https://stackify.com/best-practices-exceptions-java/
2. https://stackify.com/common-mistakes-handling-java-exception/

[sacOO7]

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java#L226

• ErrorInfo is initiated without passing a complete exception context. It just takes message out of exception and original exception cause is lost.

[sacOO7]

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/types/ErrorInfo.java#L141

• When errorInfo is initialized, the original exception cause is lost. It just created an ErrorInfo object from the exception message.
• The method shouldn't be used to initialize ErrorInfo object where the exception is caught. e.g. https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/realtime/ChannelBase.java#L365 https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/realtime/ChannelBase.java#L461

[sacOO7]

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/http/HttpCore.java#L301

• System.err is used instead of a log handler to handle the exception.

[sacOO7]

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/realtime/ChannelStateListener.java#L62

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/types/MessageSerializer.java#L74

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/realtime/Presence.java#L362

and many other places

• Exceptions are not properly logged after being caught.

[sacOO7]

https://github.com/ably/ably-java/blob/647575906f716598174f0fac3ee6a9fbb7f805d5/lib/src/main/java/io/ably/lib/http/Http.java#L49

• AblyException is caught, but ErrorInfo is passed to the callback method, so the original exception context is lost.

[sacOO7]

At some places, the only exception message is logged rather than the exception itself.

[sacOO7]

• As per https://ably-real-time.slack.com/archives/C019LDWM9UH/p1602442482002500, we were not able to find the original exception causing the error.
• At places where an exception is thrown, AblyException should be preferred over ErrorInfo to preserve the cause of the exception.

[QuintinWillison]

Thanks for looking into this, @sacOO7. As explained elsewhere (internal), this is not high priority now so is going to remain in the backlog until some of the other higher priority issues have been completed.