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

Recover from REFUSED_STREAM in HTTP/2 #2543

Closed
swankjesse opened this issue May 8, 2016 · 13 comments
Closed

Recover from REFUSED_STREAM in HTTP/2 #2543

swankjesse opened this issue May 8, 2016 · 13 comments
Labels
bug
Milestone

Comments

@swankjesse
Copy link
Member

@swankjesse swankjesse commented May 8, 2016

OkHttp promises to persevere when there’s trouble, but it doesn’t recover from REFUSED_STREAM in HTTP/2. We should transparently retry when an HTTP/2 server returns a REFUSED_STREAM error.

We should set noNewStreams = true on the connection that refused the stream. That is a good enough policy that’s in spirit of the HTTP/2 spec: it’ll prevent the connection from being used for any future streams. Then when we retry we’ll get a different connection and with any luck that one will accept the stream.

We need a special case if the refused stream’s streamId is 1. That will let us recover when Nginx refuses request bodies sent before the settings ack. In that case we should retry on the same physical connection, which we can assume has since ACK’d the settings. It’s still a little bit racy, but unlikely to be a problem in practice. More discussion on this workaround on issue 2506 and on the http-wg list.

@swankjesse swankjesse changed the title Recover from `REFUSED_STREAM` in HTTP/2 Recover from REFUSED_STREAM in HTTP/2 May 8, 2016
@swankjesse swankjesse added the bug label May 8, 2016
@swankjesse swankjesse added this to the 3.3 milestone May 8, 2016
@zyfmix

This comment has been minimized.

Copy link

@zyfmix zyfmix commented May 9, 2016

I'm okhttp3.2 with Nginx1.10.0, can you tell me how to set noNewStreams = true in my code to make it work:

public class CoamAuthInterceptor implements Interceptor {
    @Override
    public Response intercept(Chain chain) throws IOException {
        Request originalRequest = chain.request();
        Response originalResponse = chain.proceed(originalRequest);

        if (!originalResponse.isSuccessful()) {
        }

        return originalResponse;
    }
}

Thank you very much!

wmfgerrit pushed a commit to wikimedia/apps-android-wikipedia that referenced this issue May 9, 2016
OkHttp does not play nicely with nginx over HTTP/2 and this is causing
our connection to fail when trying to log in or make an edit.

This workaround will fix our issue while Square works on resolving
square/okhttp#2543 .

Bug: T134758
Bug: T134759
Change-Id: Ifc2ec3eb75e2e54b4789914f43ff9104f1ca11f9
@swankjesse

This comment has been minimized.

Copy link
Member Author

@swankjesse swankjesse commented May 11, 2016

After getting a response from the http-wg list, our recovery policy can be simpler: attempt one retry on the same connection, and then permanently fail the request.

@swankjesse

This comment has been minimized.

Copy link
Member Author

@swankjesse swankjesse commented May 11, 2016

(And do that one retry on a different connection if the current connection is shutting down.)

@dave-r12

This comment has been minimized.

Copy link
Collaborator

@dave-r12 dave-r12 commented May 11, 2016

@swankjesse if you didn't already discover this, one other thing I noted when running experiments with nginx is that even if the timing works out, it's possible OkHttp will never send the request. I traced it to this line, https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/framed/FramedConnection.java#L278

I think the headers will need to be flushed prior to writing the data frames.

@rfc2822

This comment has been minimized.

Copy link
Contributor

@rfc2822 rfc2822 commented May 15, 2016

Is this an okhttp or an nginx problem? People are starting to become nervous because clients which use okhttp (like DAVdroid) don't work with their (up-to-date) nginx servers anymore… how do you recommend to handle this? Disable HTTP2 in okhttp?

@swankjesse

This comment has been minimized.

Copy link
Member Author

@swankjesse swankjesse commented May 15, 2016

Disabling HTTP/2 in OkHttp will work. It's sad, but it'll work. Both OkHttp and Nginx are going to make code changes for this issue.

@HLFH

This comment has been minimized.

Copy link

@HLFH HLFH commented May 16, 2016

@rfc2822 I love HTTP/2. I temporarily switched to dmfs.org.

@dave-r12

This comment has been minimized.

Copy link
Collaborator

@dave-r12 dave-r12 commented May 21, 2016

A quick update: nginx attached a proposed patch on their issue tracker. I tested patch against OkHttp and it worked perfectly.

swankjesse added a commit that referenced this issue May 21, 2016
This implements the following policy:

 - If a REFUSED_STREAM error is received, OkHttp will retry the same stream
   on the same socket 1x.
 - If any other error is received, or an additional REFUSED_STREAM error is
   received, OkHttp will retry on a different route if one exists.

We may want to follow up by going through HTTP/2 error codes and deciding
a per-code retry policy, but this should be good enough for now.

Closes: #2543
swankjesse added a commit that referenced this issue May 21, 2016
This implements the following policy:

 - If a REFUSED_STREAM error is received, OkHttp will retry the same stream
   on the same socket 1x.
 - If any other error is received, or an additional REFUSED_STREAM error is
   received, OkHttp will retry on a different route if one exists.

We may want to follow up by going through HTTP/2 error codes and deciding
a per-code retry policy, but this should be good enough for now.

Closes: #2543
swankjesse added a commit that referenced this issue May 22, 2016
This implements the following policy:

 - If a REFUSED_STREAM error is received, OkHttp will retry the same stream
   on the same socket 1x.
 - If any other error is received, or an additional REFUSED_STREAM error is
   received, OkHttp will retry on a different route if one exists.

We may want to follow up by going through HTTP/2 error codes and deciding
a per-code retry policy, but this should be good enough for now.

Closes: #2543
@dangfan

This comment has been minimized.

Copy link

@dangfan dangfan commented May 22, 2016

@dave-r12 Would you please give a link of the changeset? I cannot find it in trac.nginx.org. Thanks!

@raoulbhatia

This comment has been minimized.

@rfc2822

This comment has been minimized.

Copy link
Contributor

@rfc2822 rfc2822 commented May 22, 2016

A quick update: nginx attached a proposed patch on their issue tracker. I tested patch against OkHttp and it worked perfectly.

So, should the problem go away with nginx 1.10.1 or will we still need to upgrade okhttp to get it working?

@dave-r12

This comment has been minimized.

Copy link
Collaborator

@dave-r12 dave-r12 commented May 22, 2016

@rfc2822 it seems nginx will provide a configuration option for the preread buffer. From the commit message:

If the directive's value is lower than the default initial window (65535),
as previously, all streams with data will be rejected until the new window
is acknowledged. Otherwise, no special processing is used and all requests
with data are welcome right from the connection start.

If you change it to lower than default initial window, you'll need to upgrade to 3.3. If you leave it default, everything should just work (this is the behavior most clients expect.)

I'm not familiar with their development process, so it's unclear when that change will get merged in.

@dave-r12

This comment has been minimized.

Copy link
Collaborator

@dave-r12 dave-r12 commented May 22, 2016

Also, OkHttp 3.3 will be compatible with nginx 1.9.15, if you must use that version.

swankjesse added a commit that referenced this issue May 25, 2016
This implements the following policy:

 - If a REFUSED_STREAM error is received, OkHttp will retry the same stream
   on the same socket 1x.
 - If any other error is received, or an additional REFUSED_STREAM error is
   received, OkHttp will retry on a different route if one exists.

We may want to follow up by going through HTTP/2 error codes and deciding
a per-code retry policy, but this should be good enough for now.

Closes: #2543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.