-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Non-IOException subtypes thrown from interceptor never notify Callback #5151
Comments
It works for continuations where unexpected exceptions can be caught, but I dislike it otherwise. There’s nothing reasonable to do in onFailure(). |
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. |
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:
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. |
Canceling the call is a very elegant approach. I like it a lot. Would you put the underlying exception as the cause? |
I think we don’t add it as a cause because it could get double-reported in tracking tools. |
You can add suppresses then? |
1000 :) Tried a few times to ask for something allowing handling of non IOException errors and always rejected. 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.
|
Lets do this for 4.1. |
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. |
Yuppers. |
@JakeWharton @swankjesse thanks for answer in square/retrofit#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 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 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 In #5457 says
I 100% agree with this. But I believe that if you call 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 Sorry if I'm wasting your time, just trying to help myself and other developers stumbling into this. |
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.
It still goes through the call adapter machinery. There's a test which demonstrates it that shouldn't be too hard to find. |
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? |
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. |
I was thinking more like a pluggable handling for the
I also liked it more.
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 |
A programming error in an interceptor or in OkHttp or an error of any kind will bypass the
Callback
and crash theDispatcher
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
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.The text was updated successfully, but these errors were encountered: