Skip to content
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

IOExceptions can cause infinite retries over HTTP/2 #3268

Closed
dlew opened this issue Apr 4, 2017 · 9 comments
Closed

IOExceptions can cause infinite retries over HTTP/2 #3268

dlew opened this issue Apr 4, 2017 · 9 comments
Labels
bug Bug in existing code
Milestone

Comments

@dlew
Copy link

dlew commented Apr 4, 2017

Stick the following test into HttpOverHttp2Test (which I did since it's hard to get tests to do HTTP2 otherwise):

@Test
public void errorInterceptorDoesNotInfinitelyRetry() throws Exception {
  OkHttpClient client = this.client.newBuilder()
      .addNetworkInterceptor(new Interceptor() {
        @Override
        public Response intercept(Chain chain) throws IOException {
          throw new IOException("Ouch");
        }
      })
      .build();

  Call call = client.newCall(new Request.Builder()
      .url(server.url("/foo"))
      .build());

  call.execute();
}

The test will never finish execution due to infinite retries.

The infinite retry happens in RetryAndFollowUpInterceptor. It constantly thinks it's got a recoverable error so it never quits.

As far as I can tell, the issue lies in StreamAllocation.streamFailed(). Without HTTP2 it sets route = null, which means that when you retry the request, you eventually run out of routes to try. Once StreamAllocation.hasMoreRoutes() returns false, the request is done.

However, with HTTP2 this never gets set to null, thus it always thinks it has more routes. As a result, an IOException can cause infinite retries.

@swankjesse
Copy link
Collaborator

I’d like to somehow differentiate exceptions thrown by OkHttp from exceptions thrown by interceptors and use that to enable the retry.

@swankjesse swankjesse added the bug Bug in existing code label Apr 15, 2017
@swankjesse swankjesse added this to the 3.8 milestone Apr 15, 2017
@dreamtalee
Copy link

I face the same problem now,404 also cause IOException which will cause infinite retries, i don't think it should retry infinitely in this situation. This bug only occurred when protocol is http2.

@mdsiebler
Copy link

mdsiebler commented Sep 25, 2017

Hello, I think I have a potential solution to this issue. Would it be best to discuss here or should I just submit a pull request? Thanks in advance.

I am thinking of making the following change in okhttp/src/main/java/okhttp3/internal/http/RetryAndFollowupInterceptor.java

- while ( true ) {
+ while ( followupCount++ < MAX_FOLLOW_UPS ) {

@yschimke
Copy link
Collaborator

@mdsiebler +1 to PR even if discussion continues here, would help show which other tests break (if any)

@WoodJim
Copy link

WoodJim commented Oct 11, 2017

Is my problem is some reason? I
use interceptor to retry http 2.0 request ,and server find more than 100 request at the same time ?

@NormanOu
Copy link

@swankjesse @yschimke we met this issue too. Has it been fixed? Or any suggested temporary walk around?

@swankjesse
Copy link
Collaborator

Will implement with this:
#3268

@cassgenerator
Copy link

@swankjesse The issue linked in your last comment for this fix to be implemented with is the same issue number as this issue. Is there a different issue/PR resolving this one?

@swankjesse
Copy link
Collaborator

Whoops! Will implement with this:
#3261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

No branches or pull requests

8 participants