Non-IOException subtypes thrown from interceptor never notify Callback

[JakeWharton]

A programming error in an interceptor or in OkHttp or an error of any kind will bypass the Callback and crash the Dispatcher thread leaving your consumer hanging forever. This was reported to Retrofit's Kotlin coroutine support, but is not specific to it.

My proposal is for adding something like

default void onFailure(Call call, Throwable t) {
  if (t instanceof IOException) {
    onFailure(call, (IOException) t);
  }
}

and deprecating the existing onFailure callback in a 3.14.3 or 3.15 and the equivalent added to 4.0. This will force Retrofit onto Java 8, although given the added safety I think that's acceptable.

[swankjesse]

It works for continuations where unexpected exceptions can be caught, but I dislike it otherwise. There’s nothing reasonable to do in onFailure().

[JakeWharton]

That is a dangerous assumption. Continuations are just sugar over callbacks. There's nothing specific to them that isn't also true of Rx or futures or promises or this callback. There's plenty you can do when a RuntimeException is the result of an async invocation. Imagine if the synchronous API just blocked your thread forever on anything but an IOException.

[swankjesse]

Counterproposal!

I agree that we should offer a mechanism for callers to release resources after an unexpected exception. But I don't like sending expected IOExceptions and unexpected NullPointerException/OutOfMemoryError/AssertionError failures to the same place. It risks sweeping serious bugs under the rug.

When a Call suffers an unexpected exception, let's do this:

1. Cancel the call by delivering IOException("canceled") or similar to the callback.
2. Deliver the actual failure to the UncaughtExceptionHandler.

This keeps the contract between caller and OkHttp simple: you get a response or an exception you can mostly ignore. We say, “OkHttp will cancel calls for you if there is an unexpected exception”

And it allows the real problem to be dealt with, by crashing the app or whatever has been configured.

[JakeWharton]

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

Would you put the underlying exception as the cause?

[swankjesse]

I think we don’t add it as a cause because it could get double-reported in tracking tools.

[yschimke]

You can add suppresses then?

[Tolriq]

+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)
                }
            }
        }
    }

[swankjesse]

Lets do this for 4.1.

[JakeWharton]

Any chance it can land in a 3.x for Retrofit's sake as well? I'm not quite ready to do the Kotlin dance yet.

[swankjesse]

Yuppers.

[danielesegato]

@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.

[JakeWharton]

why you can't instead just wrap the exception into an IOException and forward it like you do in the catch (e: IOException) block.

Because it's not related to I/O. You'd be implying that it's a transport problem when it's actually a programming problem.

But I believe this is not possible if you use suspend fun cause they are build-in into retrofit

It still goes through the call adapter machinery. There's a test which demonstrates it that shouldn't be too hard to find.

[danielesegato]

Because it's not related to I/O. You'd be implying that it's a transport problem when it's actually a programming problem.

yes, I know.

I liked your original proposal of just changing OkHttp onFailure interface giving it a default and deprecating the other one.

Without it's either hide the exception / crash the app outside of the call or wrap/unwrap into an IOException.

Since it looks like you can't change the interface I was hoping there was a way to wrap / unwrap the exception instead.

What about turning the behavior into something configurable in OkHttp builder so that new versions of Retrofit or other libraries could take advantage of it without breaking backward compatibility?

[JakeWharton]

I don't like the idea of a knob to change the behavior. In Retrofit we don't control the OkHttpClient instance, for example.

I think I still prefer the default method approach as well. Although given the change that was already made I have no idea if it can still be compatibly added.

[danielesegato]

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.

[yschimke]

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

cc @JakeWharton