Permit post before acking settings

[swankjesse@…]

This report is motivated by a discussion on the HTTP-WG mailing list.
​https://lists.w3.org/Archives/Public/ietf-http-wg/2016AprJun/0174.html

In OkHttp we’re investigating a bug where nginx v1.9.15 sends us
REFUSED_STREAM when we send our HEADERS and DATA frames before acking the
server’s SETTINGS.
​https://github.com/square/okhttp/issues/2506

The spec says not waiting is okay:

To avoid unnecessary latency, clients are permitted to send additional
frames to the server immediately after sending the client connection
preface, without waiting to receive the server connection preface. It is
important to note, however, that the server connection preface SETTINGS
frame might include parameters that necessarily alter how a client is
expected to communicate with the server. Upon receiving the SETTINGS frame,
the client is expected to honor any parameters established.

Not acking settings should not be the basis for refusing streams. This may
add significant latency to the initial HTTP request.

Please change nginx to accept streams created prior to the SETTINGS ack.

[Valentin V. Bartenev]

First of all, nginx doesn't reject streams prior receiving SETTINGS ack, but it rejects them without END_STREAM flag (i.e. with following DATA). This allows for a client to repeat its request without violating flow control.

Until the client has received the SETTINGS frame, it has a wrong idea about initial window size, that nginx has to change to zero.

The request body cannot be read and processed by nginx until some module asks for it. To prevent the client from sending the request body that hasn't been asked yet, the stream's window is kept zero.

[dave-r12@…]

vbart, how would I cause a module to ask for the request body? If I send a POST request with HTTP/1.1 everything works fine, but if I send it with HTTP2, the initial window size is set to 0, but is never updated. I can't seem to get nginx to send a window update to allow the POST to occur.

[Valentin V. Bartenev]

Replying to dave-r12@…:

vbart, how would I cause a module to ask for the request body?

All modules that require a request body read it by calling the ngx_http_read_client_request_body() function.

[dave-r12@…]

Replying to vbart:

Replying to dave-r12@…:

vbart, how would I cause a module to ask for the request body?

All modules that require a request body read it by calling the ngx_http_read_client_request_body() function.

Thanks, would you expect this configuration to call that function?

upstream myapp1 {
	server localhost:8080;
}
server {
	listen 443 ssl http2;
	server_name www.example.com;
	ssl on;
	ssl_certificate /root/ca.cert.pem;
	ssl_certificate_key /root/ca.key.pem;
	ssl_password_file /root/passfile;

	location / {
		proxy_pass http://myapp1;
	}
}

[Valentin V. Bartenev]

Yes, the proxy module will call it.

[dave-r12@…]

I appreciate the help vbart. I was able to get it working.

In addition to what was mentioned above, from a user perspective, these constraints are going to add more latency to the requests. From what I understand, we will now have to block on receiving the settings frame, then ack it. This introduces latency because we cannot immediately begin sending data frames. After we ack, we have to transmit the headers frame, and wait for the windows update frame before sending the data frames. Again, this introduces more latency to the request. In this case, it feels like HTTP/1.1 would actually be more efficient than HTTP2 because at least with HTTP/1.1 we can send the entire request all at once.

[Valentin V. Bartenev]

I agree, while it's not optimal from the latency point of view, such approach significantly simplifies the code. So it's a fair price, that was paid at the current stage of HTTP/2 implementation.

Introducing some kind of preread buffer will be the next step.

[dave-r12@…]

Awesome, this will be a win for the users, especially those on slow and flaky mobile networks.

[piotrsikora.google.com@…]

I think NGINX is over-aggressive here (although, still within the spec).

Per ​RFC7540, section 6.9.2. Initial Flow-Control Window Size:

Prior to receiving a SETTINGS frame that sets a value for
SETTINGS_INITIAL_WINDOW_SIZE, an endpoint can only use the default
initial window size when sending flow-controlled frames. Similarly,
the connection flow-control window is set to the default initial
window size until a WINDOW_UPDATE frame is received.

i.e. NGINX should assume clients can send 64KB before receiving SETTINGS frame, otherwise we're either:

• always forcing retry if the first request contains DATA frames,
• requiring clients to add latency by not sending requests with DATA frames before receiving SETTINGS frame from the server.

[Rogier Slag]

This seems to impact all iOS application which try to negotiate HTTP2 on nginx. One of our applications fails directly while showing "No connectivity", the other one simply crashes. Disabling http2 on those server blocks eliminates the problem

[gabor.farkas@…]

we are having this problem with iOS Safari on ios9.x. it fails when the first thing in a connection is a POST request. is there anything we could configure in nginx to somehow workaround this? for now we had to go back to http1.1 unfortunately.

[Rogier Slag]

A minimum testcase is available on ​https://github.com/inventid/http2_nginx_bug-ios

If you have XCode, you can clone this repository. Open the project using the http2_nginx_bug.xcworkspace file, then run the project on any iPhone. When you have the log open you will see the project either crashing or succeeding. (see the readme of the project for exact instructions)

[siddharth.okutech.in@…]

Is there any expected timeline for nginx 1.10.1 release? I am hoping that this bug will be fixed in 1.10.1 release.
Also is there any other work around ?

[Valentin V. Bartenev]

Please try the patch in attachment above.

[dave-r12@…]

Replying to vbart:

Please try the patch in attachment above.

I've tested with Ok Http and nghttp, both are working great! Thanks.

[Rogier Slag]

Also tested and seems to work.

Is a release to be scheduled for an nginx version 1.10.1 for this? It would be more difficult for many organizations to remain on 1.9.x (both due to the number of updates as well as the fact its not officially called "stable"). Due to the number of clients impacted by the bug, and the fact that http2 is not reliable possible using nginx for many untill 1.12 might warrant this..?

[gabor.farkas@…]

Replying to vbart:

Please try the patch in attachment above.

patch fixes my problem. (upload from ios9-web-browser)

tested on nginx checked out from todays's hg.

[Valentin Bartenev <vbart@…>]

In 6566:ce94f07d5082/nginx:

HTTP/2: implemented preread buffer for request body (closes #959).

Previously, the stream's window was kept zero in order to prevent a client
from sending the request body before it was requested (see 887cca40ba6a for
details). Until such initial window was acknowledged all requests with
data were rejected (see 0aa07850922f for details).

That approach revealed a number of problems:

1. Some clients (notably MS IE/Edge, Safari, iOS applications) show an error or even crash if a stream is rejected;

2. This requires at least one RTT for every request with body before the client receives window update and able to send data.

To overcome these problems the new directive "http2_body_preread_size" is
introduced. It sets the initial window and configures a special per stream
preread buffer that is used to save all incoming data before the body is
requested and processed.

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.

The default value is chosen to be 64k, which is bigger than the default
initial window. Setting it to zero is fully complaint to the previous
behavior.

[Valentin V. Bartenev]

Replying to rogierslag@…:

Also tested and seems to work.

Is a release to be scheduled for an nginx version 1.10.1 for this? It would be more difficult for many organizations to remain on 1.9.x (both due to the number of updates as well as the fact its not officially called "stable"). Due to the number of clients impacted by the bug, and the fact that http2 is not reliable possible using nginx for many untill 1.12 might warrant this..?

I don't know about the plans for 1.10.1 yet.

But please note that this isn't an nginx bug and the affected clients should be fixed. For everybody who uses such innovative protocol like HTTP/2, it's a good idea to stay with the latest mainline version anyway.

[waqark3389@…]

Any plans to put this patch into 1.10.2? I see 1.10.1 is already released.

[driskell@…]

Replying to waqark3389@…:

Any plans to put this patch into 1.10.2? I see 1.10.1 is already released.

I would also like to see this integrated into 1.10 branch if possible. Thanks!

[ksmithut@…]

Any updates on the ETA of the patched release?

[Maxim Dounin]

Replying to ksmithut@…:

Any updates on the ETA of the patched release?

The change in question is a part of nginx 1.11.0, released more than two months ago.

[alaz@…]

Please backport the fix into stable 1.10.x branch.

[Valentin V. Bartenev]

Replying to alaz@…:

Please backport the fix into stable 1.10.x branch.

We don't backport features to the stable branch (that's what we call stable, no enhancements). It receives only critical bug fixes. If you use such new, very complicated and actively developing protocol as HTTP/2 then it's naturally that you have to stay with the mainline branch.

See also: ​https://www.nginx.com/blog/nginx-1-10-1-11-released/

[Valentin Bartenev <vbart@…>]

In 6750:cb330cd39030/nginx:

HTTP/2: implemented preread buffer for request body (closes #959).

Previously, the stream's window was kept zero in order to prevent a client
from sending the request body before it was requested (see 887cca40ba6a for
details). Until such initial window was acknowledged all requests with
data were rejected (see 0aa07850922f for details).

That approach revealed a number of problems:

1. Some clients (notably MS IE/Edge, Safari, iOS applications) show an error or even crash if a stream is rejected;

2. This requires at least one RTT for every request with body before the client receives window update and able to send data.

To overcome these problems the new directive "http2_body_preread_size" is
introduced. It sets the initial window and configures a special per stream
preread buffer that is used to save all incoming data before the body is
requested and processed.

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.

The default value is chosen to be 64k, which is bigger than the default
initial window. Setting it to zero is fully complaint to the previous
behavior.

[kyrias@…]

For the record of anyone reading these comments, it seems that he went back on his decision, and this has in fact been included in 1.10.2 despite explicitly stated otherwise.