Catch a gson JsonIOException when parsing SimpleRequest response [SDK-1981] #355

quibi-jlk · 2020-09-25T19:39:08Z

This addresses support ticket #00467724. IOExceptions are already caught, but it turns out in some cases, gson actually wraps underlying IOExceptions in JsonIOExceptions (which does not extent IOException).

Example stack trace:

Fatal Exception: com.google.gson.JsonIOException: java.net.SocketTimeoutException: timeout
   at com.google.gson.internal.Streams.parse(Streams.java:62)
   at com.google.gson.internal.bind.TreeTypeAdapter.read(TreeTypeAdapter.java:65)
   at com.google.gson.TypeAdapter.fromJson(TypeAdapter.java:260)
   at com.auth0.android.request.internal.SimpleRequest.onResponse(SimpleRequest.java:76)
   at com.squareup.okhttp.Call$AsyncCall.execute(Call.java:177)
   at com.squareup.okhttp.internal.NamedRunnable.run(NamedRunnable.java:33)
   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
   at java.lang.Thread.run(Thread.java:764)

Caused by java.net.SocketTimeoutException: timeout
   at okio.SocketAsyncTimeout.newTimeoutException(SocketAsyncTimeout.java:143)
   at okio.AsyncTimeout.access$newTimeoutException(AsyncTimeout.java:162)
   at okio.AsyncTimeout.withTimeout(AsyncTimeout.java:154)
   at okio.AsyncTimeout$source$1.read(AsyncTimeout.java:340)
   at okio.internal.RealBufferedSourceKt.commonRead(RealBufferedSourceKt.java:39)
   at okio.RealBufferedSource.read(RealBufferedSource.java:188)
   at com.squareup.okhttp.internal.http.Http1xStream$ChunkedSource.read(Http1xStream.java:439)
   at okio.internal.RealBufferedSourceKt.commonRead(RealBufferedSourceKt.java:39)
   at okio.RealBufferedSource.read(RealBufferedSource.java:188)
   at okio.internal.RealBufferedSourceKt.commonExhausted(RealBufferedSourceKt.java:49)
   at okio.RealBufferedSource.exhausted(RealBufferedSource.java:198)
   at okio.InflaterSource.refill(InflaterSource.java:112)
   at okio.InflaterSource.readOrInflate(InflaterSource.java:76)
   at okio.InflaterSource.read(InflaterSource.java:49)
   at okio.GzipSource.read(GzipSource.java:69)
   at okio.RealBufferedSource$inputStream$1.read(RealBufferedSource.java:158)
   at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:288)
   at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:351)
   at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:180)
   at java.io.InputStreamReader.read(InputStreamReader.java:184)
   at com.google.gson.stream.JsonReader.fillBuffer(JsonReader.java:1291)
   at com.google.gson.stream.JsonReader.nextNonWhitespace(JsonReader.java:1329)
   at com.google.gson.stream.JsonReader.doPeek(JsonReader.java:550)
   at com.google.gson.stream.JsonReader.peek(JsonReader.java:426)
   at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:700)
   at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:723)
   at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:698)
   at com.google.gson.internal.Streams.parse(Streams.java:48)
   at com.google.gson.internal.bind.TreeTypeAdapter.read(TreeTypeAdapter.java:65)
   at com.google.gson.TypeAdapter.fromJson(TypeAdapter.java:260)
   at com.auth0.android.request.internal.SimpleRequest.onResponse(SimpleRequest.java:76)
   at com.squareup.okhttp.Call$AsyncCall.execute(Call.java:177)
   at com.squareup.okhttp.internal.NamedRunnable.run(NamedRunnable.java:33)
   at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
   at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
   at java.lang.Thread.run(Thread.java:764)

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.

This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not

Checklist

I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors

IOExceptions are already caught, but it turns out in some cases, gson actually wraps underlying IOExceptions in JsonIOExceptions (which does not extent IOException). Example stack trace: ``` Fatal Exception: com.google.gson.JsonIOException: java.net.SocketTimeoutException: timeout at com.google.gson.internal.Streams.parse(Streams.java:62) at com.google.gson.internal.bind.TreeTypeAdapter.read(TreeTypeAdapter.java:65) at com.google.gson.TypeAdapter.fromJson(TypeAdapter.java:260) at com.auth0.android.request.internal.SimpleRequest.onResponse(SimpleRequest.java:76) at com.squareup.okhttp.Call$AsyncCall.execute(Call.java:177) at com.squareup.okhttp.internal.NamedRunnable.run(NamedRunnable.java:33) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at java.lang.Thread.run(Thread.java:764) Caused by java.net.SocketTimeoutException: timeout at okio.SocketAsyncTimeout.newTimeoutException(SocketAsyncTimeout.java:143) at okio.AsyncTimeout.access$newTimeoutException(AsyncTimeout.java:162) at okio.AsyncTimeout.withTimeout(AsyncTimeout.java:154) at okio.AsyncTimeout$source$1.read(AsyncTimeout.java:340) at okio.internal.RealBufferedSourceKt.commonRead(RealBufferedSourceKt.java:39) at okio.RealBufferedSource.read(RealBufferedSource.java:188) at com.squareup.okhttp.internal.http.Http1xStream$ChunkedSource.read(Http1xStream.java:439) at okio.internal.RealBufferedSourceKt.commonRead(RealBufferedSourceKt.java:39) at okio.RealBufferedSource.read(RealBufferedSource.java:188) at okio.internal.RealBufferedSourceKt.commonExhausted(RealBufferedSourceKt.java:49) at okio.RealBufferedSource.exhausted(RealBufferedSource.java:198) at okio.InflaterSource.refill(InflaterSource.java:112) at okio.InflaterSource.readOrInflate(InflaterSource.java:76) at okio.InflaterSource.read(InflaterSource.java:49) at okio.GzipSource.read(GzipSource.java:69) at okio.RealBufferedSource$inputStream$1.read(RealBufferedSource.java:158) at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:288) at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:351) at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:180) at java.io.InputStreamReader.read(InputStreamReader.java:184) at com.google.gson.stream.JsonReader.fillBuffer(JsonReader.java:1291) at com.google.gson.stream.JsonReader.nextNonWhitespace(JsonReader.java:1329) at com.google.gson.stream.JsonReader.doPeek(JsonReader.java:550) at com.google.gson.stream.JsonReader.peek(JsonReader.java:426) at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:700) at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:723) at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:698) at com.google.gson.internal.Streams.parse(Streams.java:48) at com.google.gson.internal.bind.TreeTypeAdapter.read(TreeTypeAdapter.java:65) at com.google.gson.TypeAdapter.fromJson(TypeAdapter.java:260) at com.auth0.android.request.internal.SimpleRequest.onResponse(SimpleRequest.java:76) at com.squareup.okhttp.Call$AsyncCall.execute(Call.java:177) at com.squareup.okhttp.internal.NamedRunnable.run(NamedRunnable.java:33) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at java.lang.Thread.run(Thread.java:764) ```

lbalmaceda · 2020-09-30T14:50:48Z

@quibi-jlk hey 👋 looks like the actual cause was a SocketTimeoutException, which I wouldn't expect the Gson#fromJson method to throw internally as there are no more network requests at this point in the code. Although it might refer to the string stream returned by the response object being closed before time, maybe the raw content is too long?

Can you please share the steps to validate and reproduce this on our end?

quibi-jlk · 2020-09-30T15:21:58Z

@quibi-jlk hey 👋 looks like the actual cause was a SocketTimeoutException, which I wouldn't expect the Gson#fromJson method to throw internally as there are no more network requests at this point in the code. Although it might refer to the string stream returned by the response object being closed before time, maybe the raw content is too long?

Can you please share the steps to validate and reproduce this on our end?

Hi @lbalmaceda! Unfortunately we don't have a local repro on this, but we've been getting reports to crashlytics. There is one more cause wrapped by the SocketTimeoutException if that's helpful:

Caused by java.net.SocketException: socket is closed
       at com.android.org.conscrypt.ConscryptFileDescriptorSocket$SSLInputStream.read(ConscryptFileDescriptorSocket.java:554)
       at okio.InputStreamSource.read(InputStreamSource.java:90)
       at okio.AsyncTimeout$source$1.read(AsyncTimeout.java:129)
       at okio.internal.RealBufferedSourceKt.commonRead(RealBufferedSourceKt.java:39)
       at okio.RealBufferedSource.read(RealBufferedSource.java:188)
       at com.squareup.okhttp.internal.http.Http1xStream$ChunkedSource.read(Http1xStream.java:439)
       at okio.internal.RealBufferedSourceKt.commonRead(RealBufferedSourceKt.java:39)
       at okio.RealBufferedSource.read(RealBufferedSource.java:188)
       at okio.internal.RealBufferedSourceKt.commonExhausted(RealBufferedSourceKt.java:49)
       at okio.RealBufferedSource.exhausted(RealBufferedSource.java:198)
       at okio.InflaterSource.refill(InflaterSource.java:112)
       at okio.InflaterSource.readOrInflate(InflaterSource.java:76)
       at okio.InflaterSource.read(InflaterSource.java:49)
       at okio.GzipSource.read(GzipSource.java:69)
       at okio.RealBufferedSource$inputStream$1.read(RealBufferedSource.java:158)
       at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:291)
       at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:355)
       at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:181)
       at java.io.InputStreamReader.read(InputStreamReader.java:184)
       at com.google.gson.stream.JsonReader.fillBuffer(JsonReader.java:1291)
       at com.google.gson.stream.JsonReader.nextQuotedValue(JsonReader.java:1031)
       at com.google.gson.stream.JsonReader.nextString(JsonReader.java:816)
       at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:702)
       at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:723)
       at com.google.gson.internal.bind.TypeAdapters$29.read(TypeAdapters.java:698)
       at com.google.gson.internal.Streams.parse(Streams.java:48)
       at com.google.gson.internal.bind.TreeTypeAdapter.read(TreeTypeAdapter.java:65)
       at com.google.gson.TypeAdapter.fromJson(TypeAdapter.java:260)
       at com.auth0.android.request.internal.SimpleRequest.onResponse(SimpleRequest.java:76)
       at com.squareup.okhttp.Call$AsyncCall.execute(Call.java:177)
       at com.squareup.okhttp.internal.NamedRunnable.run(NamedRunnable.java:33)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
       at java.lang.Thread.run(Thread.java:919)

It does seem like it's still reading from a buffered okio stream at that point, so it will have a network connection active. It seems like this should still be caught here.

lbalmaceda · 2020-09-30T15:39:13Z

Thanks for that additional stack trace. If you happen to cross-reference and find the endpoint that was being called when this happened, please let me know. I'm tracking this internally in SDK-1981. To set expectations, I can pick this up not before the next week.

quibi-jlk · 2020-09-30T16:04:19Z

Thanks for that additional stack trace. If you happen to cross-reference and find the endpoint that was being called when this happened, please let me know. I'm tracking this internally in SDK-1981. To set expectations, I can pick this up not before the next week.

As far as I can tell, the endpoint isn't relevant. I see this both at log in (presumably hitting our log in endpoint) and post-login trying to call renewAuth. It seems like this just happens on really bad connections or networks that don't actually have an internet connection. I see other requests timing out as well, but they don't crash the app like this one does. Can you clarify why you wouldn't want to catch this exception in this case? What would be the downside of re-wrapping and properly firing this exception as a request failure instead of crashing?

lbalmaceda · 2020-10-05T14:18:04Z

What's being caught today is what is declared by the Gson#fromJson() method, which is an IOException.

Digging a bit into these issues:

JsonSyntaxException is used to wrap IOExceptions in fromJson instead of using JsonIOException google/gson#548
SocketExceptions being thrown as JsonSyntaxException in fromJson google/gson#1717 (comment)

I think the best path moving forward is to catch both IOException and JsonParseException (parent class of JsonIOException), as that would ensure future subclasses are also wrapped.

I checked the SimpleRequest class and it doesn't have tests covering the error scenarios. Would you like to add those to this PR in addition to the class name change, or do you prefer us to create a new PR with the changes?

quibi-jlk · 2020-10-05T16:15:52Z

Thanks @lbalmaceda. I've switched to a JsonParseException here. Unfortunately, I don't have the bandwidth to dig into your test suite and add new tests for SimpleRequest. Do you mind taking it from there?

lbalmaceda · 2020-10-05T23:09:03Z

Actually, it doesn't make much sense for me to approve my own changes, even if just unit tests. Let me have someone on my team check it 👍

quibi-jlk · 2020-10-05T23:14:58Z

Actually, it doesn't make much sense for me to approve my own changes, even if just unit tests. Let me have someone on my team check it 👍

Cool, thanks!!

auth0/src/test/java/com/auth0/android/request/internal/SimpleRequestTest.java

Widcket

Looks good, just left a couple of comments.

lbalmaceda · 2020-10-06T21:01:09Z

@Widcket good catch rita. I've fixed those.

lbalmaceda · 2020-10-07T15:40:05Z

@quibi-jlk I've another PR planned for the next release. If I can't get it merged by the end of the week, I'll release a patch with this change on Friday.

quibi-jlk · 2020-10-07T16:26:05Z

@quibi-jlk I've another PR planned for the next release. If I can't get it merged by the end of the week, I'll release a patch with this change on Friday.

Awesome, thanks so much @lbalmaceda!

lbalmaceda · 2020-10-09T22:05:38Z

Hey, I wanted to keep you looped. I'm postponing this release for the next week.

quibi-jlk · 2020-10-09T23:13:42Z

Hey, I wanted to keep you looped. I'm postponing this release for the next week.

Thanks for the heads up! Please let me know whenever this makes it into a release.

lbalmaceda · 2020-10-13T14:15:10Z

@quibi-jlk this was just uploaded to JCenter and should be available in Central in the next hour. Version 1.28.0.

quibi-jlk · 2020-10-13T17:04:07Z

@quibi-jlk this was just uploaded to JCenter and should be available in Central in the next hour. Version 1.28.0.

Awesome, thanks!

quibi-jlk requested a review from a team September 25, 2020 19:39

lbalmaceda added the waiting for customer This issue is waiting for a response from the issue or PR author label Sep 30, 2020

lbalmaceda added needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue and removed waiting for customer This issue is waiting for a response from the issue or PR author labels Sep 30, 2020

lbalmaceda changed the title ~~Catch a gson JsonIOException when parsing SimpleRequest response.~~ Catch a gson JsonIOException when parsing SimpleRequest response [SDK-1981] Sep 30, 2020

Switch to a JsonParseException.

14e2078

lbalmaceda added 2 commits October 5, 2020 19:58

catch JsonParseException when parsing sync requests

278af41

cover SimpleRequest with unit tests

329ab44

lbalmaceda previously approved these changes Oct 5, 2020

View reviewed changes

fix incomplete verify statement

218ef29

lbalmaceda dismissed their stale review via 218ef29 October 5, 2020 23:15

lbalmaceda added CH: Fixed and removed needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue labels Oct 5, 2020

lbalmaceda requested a review from jimmyjames October 5, 2020 23:16

lbalmaceda added this to the v1-Next milestone Oct 5, 2020

Widcket reviewed Oct 6, 2020

View reviewed changes

auth0/src/test/java/com/auth0/android/request/internal/SimpleRequestTest.java Outdated Show resolved Hide resolved

Widcket reviewed Oct 6, 2020

View reviewed changes

auth0/src/test/java/com/auth0/android/request/internal/SimpleRequestTest.java Outdated Show resolved Hide resolved

Widcket requested changes Oct 6, 2020

View reviewed changes

improve tests, match correct exception class

aa7369f

lbalmaceda requested a review from Widcket October 6, 2020 21:01

Widcket approved these changes Oct 6, 2020

View reviewed changes

lbalmaceda merged commit a419556 into auth0:master Oct 7, 2020

lbalmaceda modified the milestones: v1-Next, 1.28.0 Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch a gson JsonIOException when parsing SimpleRequest response [SDK-1981] #355

Catch a gson JsonIOException when parsing SimpleRequest response [SDK-1981] #355

quibi-jlk commented Sep 25, 2020

lbalmaceda commented Sep 30, 2020 •

edited

Loading

quibi-jlk commented Sep 30, 2020

lbalmaceda commented Sep 30, 2020

quibi-jlk commented Sep 30, 2020

lbalmaceda commented Oct 5, 2020

quibi-jlk commented Oct 5, 2020 •

edited

Loading

lbalmaceda commented Oct 5, 2020

quibi-jlk commented Oct 5, 2020

Widcket left a comment

lbalmaceda commented Oct 6, 2020

lbalmaceda commented Oct 7, 2020

quibi-jlk commented Oct 7, 2020

lbalmaceda commented Oct 9, 2020

quibi-jlk commented Oct 9, 2020

lbalmaceda commented Oct 13, 2020

quibi-jlk commented Oct 13, 2020

Catch a gson JsonIOException when parsing SimpleRequest response [SDK-1981] #355

Catch a gson JsonIOException when parsing SimpleRequest response [SDK-1981] #355

Conversation

quibi-jlk commented Sep 25, 2020

Testing

Checklist

lbalmaceda commented Sep 30, 2020 • edited Loading

quibi-jlk commented Sep 30, 2020

lbalmaceda commented Sep 30, 2020

quibi-jlk commented Sep 30, 2020

lbalmaceda commented Oct 5, 2020

quibi-jlk commented Oct 5, 2020 • edited Loading

lbalmaceda commented Oct 5, 2020

quibi-jlk commented Oct 5, 2020

Widcket left a comment

Choose a reason for hiding this comment

lbalmaceda commented Oct 6, 2020

lbalmaceda commented Oct 7, 2020

quibi-jlk commented Oct 7, 2020

lbalmaceda commented Oct 9, 2020

quibi-jlk commented Oct 9, 2020

lbalmaceda commented Oct 13, 2020

quibi-jlk commented Oct 13, 2020

lbalmaceda commented Sep 30, 2020 •

edited

Loading

quibi-jlk commented Oct 5, 2020 •

edited

Loading