-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Throwing a non-IOException from an OkHttp interceptor cancels a call without an event on the Rx stream #3453
Comments
In OkHttp we consider exceptions other than IOException to be an interceptor crash. This is because our API is originally designed for Java where the only exceptions we deliver to callbacks are IOExceptions. Your proposed fix is the right one. Subclass IOException if you want to throw from an interceptor without crashing the application. |
I just got notice of square/okhttp#5151 that explains the issue in more detail. Working around with |
Also, I was merely bumping dependencies (OkHttp from 4.6.0 to 4.8.1, Retrofit from 2.8.1 to 2.9.0 to get Rx3 support), so with the previous dependencies and Rx2 this issue did not pop up. This is weird because the aforementioned OkHttp issue was set to be included in OkHttp 4.3 already, so I wonder what else changed... |
@swankjesse should the non-IOException lead to crash in this case? |
Added some docs with my thoughts: https://github.com/square/okhttp/pull/6235/files |
I think this error is not caused by retrofit or OkHttp. I have seen several version OkHttp and Retrofit. They all not catch non-IoException derived exception, Because OkHttp only throw IOException internal. It is cased by RxJava adapter. When use @Override
protected void subscribeActual(Observer<? super Response<T>> observer) {
// Since Call is a one-shot type, clone it for each new observer.
Call<T> call = originalCall.clone();
CallCallback<T> callback = new CallCallback<>(call, observer);
observer.onSubscribe(callback);
if (!callback.isDisposed()) {
call.enqueue(callback); // <---------not catch other exception.
}
} You can use val retrofit = Retrofit.Builder()
.client(okhttp)
.addCallAdapterFactory(RxJava3CallAdapterFactory.createSynchronous())
// Or
// .addCallAdapterFactory(RxJava3CallAdapterFactory.createWithScheduler(Schedulers.io()))
.baseUrl(server.url("/"))
.build() It will use protected void subscribeActual(Observer<? super Response<T>> observer) {
...
try {
Response<T> response = call.execute();
...
} catch (Throwable t) {
Exceptions.throwIfFatal(t);
if (terminated) {
...
} else if (!disposable.isDisposed()) {
try {
observer.onError(t);
} catch (Throwable inner) {
...
}
}
}
} |
@YaowenGuo Very good overview, indeed! Now the question is: If any user picks any of the factory methods ( |
Also, I think the real culprit is really @Override protected void execute() {
boolean signalledCallback = false;
transmitter.timeoutEnter();
try {
Response response = getResponseWithInterceptorChain();
signalledCallback = true;
responseCallback.onResponse(RealCall.this, response);
} catch (IOException e) {
if (signalledCallback) {
// Do not signal the callback twice!
Platform.get().log(INFO, "Callback failure for " + toLoggableString(), e);
} else {
responseCallback.onFailure(RealCall.this, e);
}
} catch (Throwable t) {
cancel(); // <-- this is IMHO too early
if (!signalledCallback) {
IOException canceledException = new IOException("canceled due to " + t);
canceledException.addSuppressed(t);
responseCallback.onFailure(RealCall.this, canceledException);
}
throw t;
} finally {
client.dispatcher().finished(this);
} and here of the @Override
public void onFailure(Call<T> call, Throwable t) {
if (call.isCanceled()) return; // <-- yeah, we're canceled and do not process the error, leading to an indefinite open stream
try {
observer.onError(t);
} catch (Throwable inner) {
Exceptions.throwIfFatal(inner);
RxJavaPlugins.onError(new CompositeException(t, inner));
}
} |
Yes, I also confused by this difference。I also its behavior will be consistent for downstream. |
I ran into this problem yesterday via a suspending function in a Retrofit interface. I was extremely confused because I was able to try/catch the suspending function call, and catch the exception, but the app still crashed at ReallCall.AsyncCall.run. It makes sense now why this happens, but it took a full day of digging and debugging to figure it out. @swankjesse's updated docs help a lot. The current docs for Interceptor don't explain this at all. I would like to suggest two more things to make it easier for developers to figure out that only descendants of IOException are expected. First, It would be good to have a log statement in the Platform.get().log("Interceptor error for ${toLoggableString()}. Interceptors are only expected to throw subclasses of IOException.", Platform.INFO, t) Second, it would be nice if it wrapped the unexpected error, and gave it more detail. e.g. throw IllegalStateException("Unexpected error thrown by Interceptor. Only expected IOException and descendants",t) This would still allow the app to crash, but would at least tell developers exactly why they're seeing a stack trace that doesn't point to the call site of the My only concern with the second suggestion is whether changing the exception that is thrown is likely to break things for consumers of the API. |
If a non-
IOException
-derived exception is thrown from an OkHttp interceptor, an RxJava3 call adapter stalls indefinitely and does not signal the exception downstream. (This used to work with the RxJava2 call adapter.)This is a quick reproducer: https://gist.github.com/realdadfish/c7c0d8cb123c583acd3e683b68afbfa5
The current workaround is to simply make all custom exceptions thrown from within interceptors derive from
IOException
.Retrofit: 2.9.0
OkHttp: 4.8.1
RxJava: 3.0.6
The text was updated successfully, but these errors were encountered: