okhttp: Non-IOException subtypes thrown from interceptor never notify Callback

+1000 😃 Tried a few times to ask for something allowing handling of non IOException errors and always rejected. Currently using added interceptor for that …

Typical use on Android would be security issues for rooted user who by default block Internet permission, not crashing and having the exception allows to properly notify the user.

    private class OkHttpExceptionInterceptor : Interceptor {

        @Throws(IOException::class)
        override fun intercept(chain: Interceptor.Chain): Response {
            try {
                return chain.proceed(chain.request())
            } catch (e: Throwable) {
                if (e is IOException) {
                    throw e
                } else {
                    throw IOException(e)
                }
            }
        }
    }

I don’t like the idea of a knob to change the behavior.

I was thinking more like a pluggable handling for the catch(Throwable) part with a default implementation.

I think I still prefer the default method approach as well.

I also liked it more.

Although given the change that was already made I have no idea if it can still be compatibly added.

the default implementation could just rethrow and an external catch would do exactly what it does now.

as an alternative you could add a new interface and just check for it and use it if available.

I don’t think there’s a clean way to make this kind of change in an API without breaking backward compatibility or requiring an extra step.

@swankjesse if we want to do this, we should do it for 5.x, or close.

cc @JakeWharton

@JakeWharton @swankjesse thanks for answer in https://github.com/square/retrofit/issues/3505

This is currently the implementation in okhttp

    override fun run() {
      threadName("OkHttp ${redactedUrl()}") {
        var signalledCallback = false
        timeout.enter()
        try {
          val response = getResponseWithInterceptorChain()
          signalledCallback = true
          responseCallback.onResponse(this@RealCall, response)
        } catch (e: IOException) {
          if (signalledCallback) {
            // Do not signal the callback twice!
            Platform.get().log("Callback failure for ${toLoggableString()}", Platform.INFO, e)
          } else {
            responseCallback.onFailure(this@RealCall, e)
          }
        } catch (t: Throwable) {
          cancel()
          if (!signalledCallback) {
            val canceledException = IOException("canceled due to $t")
            canceledException.addSuppressed(t)
            responseCallback.onFailure(this@RealCall, canceledException)
          }
          throw t
        } finally {
          client.dispatcher.finished(this)
        }
      }
    }

the catch(t: Throwable) part is what I’m not happy about, but I admit I don’t fully understand it. Like for instance I do not understand why you call cancel() there and why you can’t instead just wrap the exception into an IOException and forward it like you do in the catch (e: IOException) block.

I suppose if you use OkHttp directly it’s not a big issue, you are writing the code for each Http request and can easily wrap/unwrap exception yourself

I think this is more an issue at the Retrofit level than of OkHttp level: wrapping the Interceptor is easier than wrapping a retrofit service that is auto-generated / handled by retrofit.

With RxJava2 retrofit interface:

interface MyApi {
  @GET("whatever")
  fun get(): Single<Response<MyBody>>
}

if an interceptor throw an exception that is not IOException the Single get canceled and the onFailure callback ignored.

which means that if an Rx observer is waiting for something on it it will never receive a callback and will stay hanging there, the error is not even logged.

Using kotlin coroutines the behavior is different

interface MyApi {
  @GET("whatever")
  suspend fun get(): Response<MyBody>
}

In this case the coroutines is, again, canceled but the app crashes with the exception

I believe the reason is here:

suspend fun <T> Call<T>.awaitResponse(): Response<T> {
  return suspendCancellableCoroutine { continuation ->
    continuation.invokeOnCancellation {
      cancel()
    }
    enqueue(object : Callback<T> {
      override fun onResponse(call: Call<T>, response: Response<T>) {
        continuation.resume(response)
      }

      override fun onFailure(call: Call<T>, t: Throwable) {
        continuation.resumeWithException(t)
      }
    })
  }
}

there no isActive check in onFailure before calling resumeWithException(t) and since the coroutine has been already canceled here it crash the app.

In https://github.com/square/okhttp/pull/5457 says

It would also be nice if integrations could unwrap this. What about using a specific subtype of IOException so the Rx and Coroutine code in Retrofit can correctly unwrap this and propagate the real, crash-worthy cause?

I 100% agree with this.

But I believe that if you call cancel() there’s no way for the exception to reach retrofit adapters (and have a chance to be unwrapped)

I believe the minimal impact option for a developer right now is to write a custom CallFactory that wraps rebuild an OkHttpClient instance wrapping every interceptor into a Delegate interceptor that just catches and wrap non-IOExceptions, than use a custom CallAdapter to do the unwrapping.

But I believe this is not possible if you use suspend fun cause they are build-in into retrofit. So the only other option I could think of is to manually unwrap exceptions above the retrofit service or writing a Proxy, except I’m not sure if there is any implication of writing a Proxy of a Kotlin class.

Sorry if I’m wasting your time, just trying to help myself and other developers stumbling into this.

Canceling the call is a very elegant approach. I like it a lot.

Would you put the underlying exception as the cause?