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

okhttp 3.4.1will go endless loop,when there is a IOException("shutdown") #2756

Closed
leokelly opened this Issue Jul 28, 2016 · 20 comments

Comments

8 participants
@leokelly

leokelly commented Jul 28, 2016

here is how it go endless loop:
in file RetryAndFollowUpInterceptor.java

public Response intercept(Chain chain) throws IOException {

Request request = chain.request();
streamAllocation = new StreamAllocation(
    client.connectionPool(), createAddress(request.url()));
int followUpCount = 0;
Response priorResponse = null;
while (true) {
  if (canceled) {
    streamAllocation.release();
    throw new IOException("Canceled");
  }

  Response response = null;
  boolean releaseConnection = true;
  try {
    response = ((RealInterceptorChain) chain).proceed(request, streamAllocation, null, null);
    releaseConnection = false;
  } catch (RouteException e) {
    // The attempt to connect via a route failed. The request will not have been sent.
    if (!recover(e.getLastConnectException(), true, request)) throw e.getLastConnectException();
    releaseConnection = false;
    continue;
  } catch (IOException e) {
    // An attempt to communicate with a server failed. The request may have been sent.
    if (!recover(e, false, request)) throw e;
    releaseConnection = false;
    continue;
  } finally {
    // We're throwing an unchecked exception. Release any resources.
    if (releaseConnection) {
      streamAllocation.streamFailed(null);
      streamAllocation.release();
    }
  }

when there is a IOException("shutdown") when execute
response = ((RealInterceptorChain) chain).proceed(request, streamAllocation, null, null);
it will go into catch block,and judge if can recover,recover(e, false, request) unfortunately recover() return true,so it will continue and go endless loop

@swankjesse

This comment has been minimized.

Member

swankjesse commented Jul 28, 2016

Can you provide a test case to demonstrate?

@swankjesse swankjesse added this to the 3.5 milestone Jul 28, 2016

@mopsalarm

This comment has been minimized.

mopsalarm commented Aug 27, 2016

I had the same issue today too. The loop ran endlessly for minutes until I killed the app. Maybe put an upper bound to the number of executions or at least slow down after 10 iterations?

@swankjesse

This comment has been minimized.

Member

swankjesse commented Aug 28, 2016

We gotta make sure shutdown connections get removed from the pool.

@dave-r12

This comment has been minimized.

Collaborator

dave-r12 commented Sep 11, 2016

@swankjesse were you planning to pick this one up? If not I can look into it.

@swankjesse

This comment has been minimized.

Member

swankjesse commented Sep 12, 2016

Please do!

@fabioCollini

This comment has been minimized.

fabioCollini commented Sep 14, 2016

I have a similar issue using http2, I tried to debug the code and I have found this TODO in method isHealthy of RealConnection class:

if (framedConnection != null) {
  return true; // TODO: check framedConnection.shutdown.
}

The problem seems to be related to this TODO.

@dave-r12

This comment has been minimized.

Collaborator

dave-r12 commented Sep 14, 2016

@fabioCollini yup, although that snippet is an older version. I'm working on a fix.

@nsk-mironov

This comment has been minimized.

nsk-mironov commented Sep 30, 2016

Is there any update on this issue? We're running into the same issue in our project. After some investigation we've discovered that the following code might not work as it's expected to work StreamAllocation.java#L135-L139:

synchronized (connectionPool) {
  if (candidate.successCount == 0) {
    return candidate;
  }
}

candidate.successCount == 0 returns true, but candidate.isHealthy(doExtensiveHealthChecks) returns false for the same connection instance.

I don't have a test case at the moment, but I'll try to find and provide it ASAP.

@dave-r12

This comment has been minimized.

Collaborator

dave-r12 commented Oct 1, 2016

Yea, sorry I fell behind on this. The test case is straightforward, send a GOAWAY frame without closing the connection. I'll try to wrap this up in the next day or so.

dave-r12 added a commit that referenced this issue Oct 3, 2016

dave-r12 added a commit that referenced this issue Oct 8, 2016

dave-r12 added a commit that referenced this issue Oct 8, 2016

dave-r12 added a commit that referenced this issue Oct 8, 2016

dave-r12 added a commit that referenced this issue Oct 12, 2016

dave-r12 added a commit that referenced this issue Oct 13, 2016

dave-r12 added a commit that referenced this issue Oct 13, 2016

dave-r12 added a commit that referenced this issue Oct 16, 2016

dave-r12 added a commit that referenced this issue Oct 16, 2016

dave-r12 added a commit that referenced this issue Oct 16, 2016

dave-r12 added a commit that referenced this issue Oct 16, 2016

dave-r12 added a commit that referenced this issue Oct 16, 2016

swankjesse added a commit that referenced this issue Oct 16, 2016

swankjesse added a commit that referenced this issue Oct 16, 2016

swankjesse added a commit that referenced this issue Oct 16, 2016

dave-r12 added a commit that referenced this issue Oct 16, 2016

swankjesse added a commit that referenced this issue Oct 16, 2016

@dlew

This comment has been minimized.

dlew commented Oct 24, 2016

We're blocked on releasing due to this issue, so I'm trying to help out where I can... I'm not sure if this will help at this point, but here's a demonstration of the problem: dlew@5092a7b

@kenyee

This comment has been minimized.

kenyee commented Nov 3, 2016

bit by this too...hope it gets released soon :-)

@kenyee

This comment has been minimized.

kenyee commented Nov 3, 2016

What version was this bug introduced in? I.e., can we switch to an older version of okhttp to work around it?

Looks like the answer is no. I went back to 3.2.0 (couldn't go further w/o hitting conflicts w/ other libraries) and it's still there. It's easy to reproduce in an app by using CharlesProxy and throttling w/ a 50% reliability network.

swankjesse added a commit that referenced this issue Nov 3, 2016

@swankjesse

This comment has been minimized.

Member

swankjesse commented Nov 3, 2016

I’m hoping to cherrypick #2955 and release it as 3.4.2.

#2955

@swankjesse

This comment has been minimized.

Member

swankjesse commented Nov 4, 2016

Released as 3.4.2.

@kenyee

This comment has been minimized.

kenyee commented Nov 4, 2016

Thanks for the quick release @swankjesse...that fixed a weird issue w/ our app where a bad network caused a timeout and then the network stack fell over and the user had to restart the app to fix it :-)

@kenyee

This comment has been minimized.

kenyee commented Nov 4, 2016

From more testing, I found that if the network gets crappy enough (if you leave CharlesSSL at 50% and keep trying to get your app to load stuff over the network when Charles gives you an unstable network), the network stack gets stuck w/ these still:
Caused by: java.io.IOException: shutdown
at okhttp3.internal.framed.FramedConnection.newStream(FramedConnection.java:238)
Retrofit (1.9.0 if it matters) then gets stuck as before...takes longer now so initial testing didn't show this...the fix definitely helped.

@swankjesse

This comment has been minimized.

Member

swankjesse commented Nov 4, 2016

@kenyee yeah, I know and I’m sad about that. We’ve got a couple of failing tests where our ‘perseverence’ goals aren’t being met. We’ll fix those before closing this issue.

@swankjesse

This comment has been minimized.

Member

swankjesse commented Nov 20, 2016

@dave-r12 anything left here?

@dave-r12

This comment has been minimized.

Collaborator

dave-r12 commented Nov 20, 2016

Nothing I can think of.

@swankjesse swankjesse closed this Nov 20, 2016

@swankjesse

This comment has been minimized.

Member

swankjesse commented Nov 20, 2016

Sweet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment