Opened 19 months ago

Closed 18 months ago

Last modified 12 months ago

#959 closed enhancement (fixed)

Permit post before acking settings

Reported by: swankjesse@… Owned by: Valentin Bartenev <vbart@…>
Priority: minor Milestone:
Component: nginx-core Version: 1.9.x
Keywords: Cc:
Sensitive: no
uname -a: Darwin jwilson.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016;
nginx -V: 1.9.15

Description

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.

Attachments (1)

preread_buffer.patch (13.6 KB) - added by vbart 18 months ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 19 months ago by vbart

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.

comment:2 follow-up: Changed 19 months ago by 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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 19 months ago by 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.

comment:4 in reply to: ↑ 3 Changed 19 months ago by 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;
	}
}

comment:5 Changed 19 months ago by vbart

Yes, the proxy module will call it.

comment:6 Changed 19 months ago by 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.

comment:7 Changed 19 months ago by vbart

  • Status changed from new to accepted
  • Type changed from defect to enhancement

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.

comment:8 Changed 19 months ago by dave-r12@…

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

comment:9 Changed 19 months ago by 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.

comment:10 Changed 18 months ago by rogierslag@…

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

comment:11 Changed 18 months ago by 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.

comment:12 Changed 18 months ago by rogierslag@…

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)

Last edited 18 months ago by rogierslag@… (previous) (diff)

comment:13 Changed 18 months ago by 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 ?

Changed 18 months ago by vbart

comment:14 follow-ups: Changed 18 months ago by vbart

Please try the patch in attachment above.

comment:15 in reply to: ↑ 14 Changed 18 months ago by 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.

Last edited 18 months ago by dave-r12@… (previous) (diff)

comment:16 follow-up: Changed 18 months ago by 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..?

comment:17 in reply to: ↑ 14 Changed 18 months ago by 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.

comment:18 Changed 18 months ago by Valentin Bartenev <vbart@…>

  • Owner set to Valentin Bartenev <vbart@…>
  • Resolution set to fixed
  • Status changed from accepted to closed

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;
  1. 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.

comment:19 in reply to: ↑ 16 Changed 18 months ago by vbart

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.

comment:20 follow-up: Changed 18 months ago by waqark3389@…

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

comment:21 in reply to: ↑ 20 Changed 18 months ago by 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!

comment:22 follow-up: Changed 16 months ago by ksmithut@…

Any updates on the ETA of the patched release?

comment:23 in reply to: ↑ 22 Changed 16 months ago by mdounin

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.

comment:24 follow-up: Changed 16 months ago by alaz@…

Please backport the fix into stable 1.10.x branch.

comment:25 in reply to: ↑ 24 Changed 16 months ago by vbart

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/

comment:26 Changed 13 months ago by 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;
  1. 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.

comment:27 Changed 12 months ago by 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.

Note: See TracTickets for help on using tickets.