implement http error handling #119

pb82 · 2018-03-01T09:16:33Z

Motivation

Allow better error handling when using the http module and fix a bug in the metrics module. When the return code from a request was 500 the onComplete handler was triggered. The metrics module tried to evaluate the requestError but it was null. The current implementation does set the requestError only when the request itself failed, not when the response returned an invalid status code.

Example:

final HttpResponse resp = httpRequest.execute();
resp.onSuccess(() -> {
  // Do something with the result           
}).onError(() -> {
  // Print an error message, you can safely access resp.getRequestError()
}).onComplete() -> {
  // Close any resources, popups... This will always be called
});

Description

Adds onError and onSuccess handlers to the HttpResponse objects. I've looked at the OkHttp recipes (https://github.com/square/okhttp/wiki/Recipes) and they seem to suggest a similar pattern for error handling.

Tested with:

successful request
request that returns status 500
request to invalid URL

Progress

Done Task

david-martin · 2018-03-01T09:21:56Z

@pb82 Looks good. Do we use this pattern elsewhere?
Will leave approval to @danielpassos & @secondsun

pb82 · 2018-03-01T09:36:26Z

@david-martin it's the success/error callback pattern that's widely used in async languages. Not sure about Android, but since we already have onComplete i think it makes sense to also have onError.

The problem with onComplete was that you can't be sure that the requestError object was set and before using it you always had to check if it is null (If the server returned 500 the error was not set, only when the request failed completely).

Now when onError is called there will always be a requestError that you can inspect. In the case of a 500 it will look like:

E/AeroGear: Metrics request error java.io.IOException: Request failed with status code 500

There's another use in the auth module where the error handler also could be used: https://github.com/aerogear/aerogear-android-sdk/blob/master/auth/src/main/java/org/aerogear/mobile/auth/credentials/JwksManager.java#L101

(But the old method also still works)

pb82 · 2018-03-01T10:22:59Z

core/src/main/java/org/aerogear/mobile/core/metrics/publisher/NetworkMetricsPublisher.java

-                }
+                MobileCore.getLogger().debug("Metrics sent: " + json.toString());
+            }).onError(() -> {
+                MobileCore.getLogger().error("Metrics request error",


@secondsun @danielpassos @wtrocki Should we log anything here? This will happen on devices out in the field, while the logs are only useful when plugged into Android Studio. Are you aware of any conventions about logging in Android apps?

Great to see this example aligned with IOS:
https://github.com/aerogear/aerogear-ios-sdk/blob/master/modules/core/metrics/engine/MetricsNetworkPublisher.swift#L20-L26

This can be ok for now. For the cases when network connection is down error will be printed but that's not a big problem IMHO.
I have no strong opinion about it, but would like to keep that aligned with IOS no matter what way we would go.

Actually this area may need more work anyway. We could handle 401/403 errors if token is wrong and print different error.

wtrocki

Switched to your branch. Reviewed and verified.

josemigallas · 2018-03-01T10:36:26Z

core/src/test/java/org/aerogear/mobile/core/http/OkHttpServiceModuleTest.java

@@ -43,4 +43,26 @@ public void testGet() {
        response.waitForCompletionAndClose();
    }

+    @Test
+    public void testErrorHandler() {


Please @pb82 rename this method accordingly with what the test case is about

But it is about testing the error handler?

josemigallas · 2018-03-01T10:40:21Z

core/src/test/java/org/aerogear/mobile/core/http/OkHttpServiceModuleTest.java

+        assertNotNull(response);
+
+        response.onComplete(() -> {
+            // The complete handler must not be called here


Call Assert.fail(); instead of comment plus forced bad assertion

danielpassos · 2018-03-01T11:59:34Z

core/src/main/java/org/aerogear/mobile/core/http/OkHttpResponse.java

    @Override
    public HttpResponse onComplete(Runnable completionHandler) {
        this.completionHandler = completionHandler;
-        if (response != null) {
+        if (response != null && response.isSuccessful()) {


I think we should have any if here. I really don't care if there is a response or it was a success.

Think with me... I have raised a loading popup and wanna dismiss it when the request is complete. Complete should be called always independent of the success or fail.

At the end of the day, request should be always called runCompletionHandler + (runCompletionHandler || runErrorHandler)

It that makes sense?

Ok, understand. Then you would see the completion handler more as soemthing like a finally? I'm ok with handling that in the completion handler but then we should probably remove the error handler because you would have to handle a possible error in your completion handler anyway.

danielpassos · 2018-03-01T12:04:17Z

core/src/main/java/org/aerogear/mobile/core/http/OkHttpResponse.java

+    @Override
+    public HttpResponse onError(Runnable errorHandler) {
+        this.errorHandler = errorHandler;
+        if (response != null && !response.isSuccessful()) {


I'm not sure if we need this kind of check here. IMO we should check only if errorHandler is null to avoid NPE

This check is required because otherwise it could happen that you attach the errorHandler when the network thread has not yet finished and response is still null. In that case we can not yet check if the response was successful and have to wait for the network thread callback to handle that.

danielpassos · 2018-03-01T12:05:47Z

core/src/main/java/org/aerogear/mobile/core/http/OkHttpResponse.java

@@ -66,8 +87,10 @@ public void waitForCompletionAndClose() {
            requestCompleteLatch.await(DEFAULT_TIMEOUT, TimeUnit.SECONDS);
            //If a completion Handler was set before this wait then we need to make sure that
            // gets called before we free these resources.
-            if (completionHandler != null) {
+            if (completionHandler != null && requestError == null) {


Like I said before if we have a completionHandler it should be always called, not only on success

Hmm, wouldn't that make the error handler pointless? Because then you would have to check for errors in your completion handler.

@danielpassos I was thinking about the same, but if you read the javadoc of the onComplete method:

/**
* Creates a callback to be called when the response is finished successfully.
*
* The response is delivered immediately if the request has already been finished.
*/

It says that it is only called when the response is finished successfully.

If this is still the case, then I think it shouldn't be called when there is an error.

I think the name of the method is a bit confusing. I agree that if it is called onComplete, then it should be called all the time. Otherwise it's better rename it to onSuccess.

@wei-lee i'll update this PR soon to include onSuccess, onError and onComplete. Complete will always be called, success only when the request was successful.

matzew · 2018-03-01T14:41:56Z

core/src/main/java/org/aerogear/mobile/core/http/HttpResponse.java

+     * @param runnable a callback method
+     * @return the instance of the HttpResponse for API chaining
+     */
+    HttpResponse onSuccess(Runnable runnable);


doesn't this pretty much make the onComplete redundant ?

I'd assume it's no a "long-running" response, right ?

@matzew it's like try, catch, finally. onComplete can always be used to clean things up even when there's an error. And you would use onSuccess only if the request succeeded to do things that rely on the data.

@matzew Nopz, it's the same way RxJava handler stuffs

http://reactivex.io/RxJava/2.x/javadoc/io/reactivex/Observer.html

@danielpassos actually, nope - Flowable/Observable are emitting n objects - while Single just emits one. See the subscribe(Consumer<? super T> onSuccess, Consumer<? super Throwable> onError)

that's why I asked if these are long-running requests, where something like onNext(ByteBuffer) would make sense.

--

Right now this API doesn't support that type of request. When the request gets done things get rolled into memory for better or worse.

danielpassos · 2018-03-01T15:18:35Z

core/src/main/java/org/aerogear/mobile/core/http/OkHttpResponse.java

-                    runCompletionHandler();
+
+                if (!response.isSuccessful()) {
+                    throw new IOException("Request failed with status code " + response.code());


I'm in doubt what we should do here. The request was success called, but the response was not (range [200..300)). This should be a dev responsibility to handle that. IMO we should always call the success expect when the HTTP layer raises a problem.

@secondsun Can you give your opinion here?

So first a bit of bikeshedding, IOException is the wrong exception to use here. IOException is more of a "the network quit working" type of failure.

I think that success and error handlers make a lot more sense when we start thinking about ways for modules to "rescue" a http request. If auth can refresh a token then it should be able to register as an error handler and then this distinction makes a lot of sense.

However this kind of changes the semantics a bit. A request is made. A response is returned. success and error handlers are called (which may or may not generate other network activity as appropriate) and when the whole mess has settled the completion handler has finally been called. Completion therefore stops being about a single HTTP request and is instead about a user's action in the application.

Sorry I don't think I made anything more clear LOL

pb82 · 2018-03-01T16:35:38Z

@secondsun I agree with you that IOException is the wrong type for a invalid response even though the OkHttp guys seem to bee happy with it: https://github.com/square/okhttp/blob/master/samples/guide/src/main/java/okhttp3/recipes/SynchronousGet.java#L33

I've changed that to a custom Exception.

@secondsun @matzew @wei-lee I was discussing this change with @danielpassos and there is one open question about the onSuccess/onError handlers:

Should a response with an error code (e.g. 500) be treated as an error (onError) or treated as a successful request & response (onSuccess) and we only call onError for Network errors?

I'd prefer to call onError for a 500 response but i'm happy to change it if you think that is wrong.

What's your Opinion on this?

wei-lee · 2018-03-01T16:49:47Z

@pb82 I agree with you and I think it should call onError for 5xx responses. In this case, there should be a response object and the developer can use the status code to determine the type of the error.

secondsun · 2018-03-01T21:38:47Z

core/src/main/java/org/aerogear/mobile/core/http/HttpResponse.java

    /**
     * Returns the request error if it failed
     * @return Exception request error or null
     */
-    Exception getRequestError();
+    Exception getHttpError();


How about just getError?

secondsun · 2018-03-01T21:39:16Z

core/src/main/java/org/aerogear/mobile/core/http/InvalidResponseException.java

+ * Custom exception thrown when a http request returns an unenxpected
+ * result or result code
+ */
+public class InvalidResponseException extends Exception {


I would prefer extending runtimeexception.

tomjackman · 2018-03-02T14:36:09Z

@pb82 Are HTTP 302 Redirects considered errors in a response? I've updated my PR to use the changes here, but it's causing issues with Keycloak, specifically on Keycloak logouts where the result is a HTTP 302 for a successful logout.

E/AeroGear: Error Performing a Logout on the Remote OIDC Server: 
            org.aerogear.mobile.core.http.InvalidResponseException: Unexpected response from server. Status code was: 302
                at org.aerogear.mobile.core.http.OkHttpResponse.lambda$new$0$OkHttpResponse(OkHttpResponse.java:30)

pb82 · 2018-03-02T14:41:12Z

@TommyJ1994 Apparently yes: https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/Response.java#L106

pb82 · 2018-03-02T14:43:48Z

@TommyJ1994 There are three possible ways to resolve this:

we provide our own isSuccesful that includes redirects
we go with @danielpassos approach and consider every response a success and only network errors as failures
we add a onRedirect handler. Probably not a good idea.

secondsun · 2018-03-02T14:49:40Z

I vote for 2

tomjackman · 2018-03-02T14:53:41Z

2 for me too for flexibility.

pb82 · 2018-03-02T14:54:54Z

@secondsun @TommyJ1994 Ok, preparing a PR.

tomjackman · 2018-03-02T14:58:51Z

@pb82 Created an issue for this so you can have a reference for the PR. #123

danielpassos · 2018-03-05T23:24:33Z

2 is exactly what Okhttp do, they do this because there is a difference between request success and response success. We should handle only the request because there is the lib responsibility. If we could fire the request without any problem we should call success, if not we should call error. Handle the response error is the dev responsibility.

pb82 requested review from danielpassos, david-martin, wei-lee, wtrocki, secondsun and josemigallas March 1, 2018 09:16

pb82 force-pushed the http-error-handling branch from 027eade to 63f3ff6 Compare March 1, 2018 10:20

pb82 commented Mar 1, 2018

View reviewed changes

pb82 force-pushed the http-error-handling branch 2 times, most recently from c82f277 to 9a1ee34 Compare March 1, 2018 10:31

wtrocki approved these changes Mar 1, 2018

View reviewed changes

wei-lee approved these changes Mar 1, 2018

View reviewed changes

josemigallas reviewed Mar 1, 2018

View reviewed changes

pb82 force-pushed the http-error-handling branch 2 times, most recently from 9bfe841 to 9e302ba Compare March 1, 2018 11:17

josemigallas approved these changes Mar 1, 2018

View reviewed changes

danielpassos suggested changes Mar 1, 2018

View reviewed changes

pb82 force-pushed the http-error-handling branch from fa242d9 to f5cb607 Compare March 1, 2018 14:13

matzew reviewed Mar 1, 2018

View reviewed changes

wei-lee approved these changes Mar 1, 2018

View reviewed changes

danielpassos reviewed Mar 1, 2018

View reviewed changes

pb82 force-pushed the http-error-handling branch from f5cb607 to 3ad7ecb Compare March 1, 2018 16:21

secondsun reviewed Mar 1, 2018

View reviewed changes

pb82 force-pushed the http-error-handling branch from 3ad7ecb to cc02fe9 Compare March 2, 2018 09:17

danielpassos approved these changes Mar 2, 2018

View reviewed changes

pb82 added 3 commits March 2, 2018 14:23

implement http error handling

994f6a1

cleanup

83233e8

add onSuccess handler and run onComplete always

5a1143d

pb82 force-pushed the http-error-handling branch from cc02fe9 to 5a1143d Compare March 2, 2018 13:23

pb82 merged commit 1f72c69 into aerogear:master Mar 2, 2018

pb82 deleted the http-error-handling branch March 2, 2018 14:00

implement http error handling #119

implement http error handling #119

Conversation

pb82 commented Mar 1, 2018 • edited

Motivation

Description

Progress

david-martin commented Mar 1, 2018

pb82 commented Mar 1, 2018

Choose a reason for hiding this comment

wtrocki Mar 1, 2018 • edited

Choose a reason for hiding this comment

wtrocki Mar 1, 2018 • edited

Choose a reason for hiding this comment

wtrocki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pb82 Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpassos Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pb82 commented Mar 1, 2018

wei-lee commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjackman commented Mar 2, 2018

pb82 commented Mar 2, 2018

pb82 commented Mar 2, 2018

secondsun commented Mar 2, 2018

tomjackman commented Mar 2, 2018

pb82 commented Mar 2, 2018

tomjackman commented Mar 2, 2018

danielpassos commented Mar 5, 2018 • edited

pb82 commented Mar 1, 2018 •

edited

wtrocki Mar 1, 2018 •

edited

wtrocki Mar 1, 2018 •

edited

pb82 Mar 1, 2018 •

edited

danielpassos Mar 1, 2018 •

edited

danielpassos commented Mar 5, 2018 •

edited