search

openresty / lua-nginx-module

Embed the Power of Lua into NGINX HTTP servers
https://openresty.org/
11.24k stars 2.02k forks source link

HTTP/2 request processing optionally #2211

Open splitice opened 1 year ago

splitice commented 1 year ago

PR https://github.com/openresty/lua-nginx-module/pull/2174 removes support for http request processing.

I think this should be optionally still supported, it is after all perfectly servisable in a HTTP/2 request response flow. Perhaps a timeout can be added to error on grpc (rather than spiking the request)?

oowl commented 1 year ago

The problem is not only GRPC requests, All HTTP2 traffic may have long connection stream, you can see my analyze in https://github.com/openresty/lua-nginx-module/issues/2172#issuecomment-1486378063

oowl commented 1 year ago

HTTP2 is stream base protocol, read the whole body is theoretically impossible, and it does not make sense.

splitice commented 1 year ago

The way nginx does it, potentially ignoring Content-Length seems a bit strange. However most request / responses do follow the non infinite upload property...

I think a timeout would be a good idea (maximum time spent reading request data) for sure.

oowl commented 1 year ago

I think a timeout would be a good idea (maximum time spent reading request data) for sure.

I agree 😆 cc @zhuizhuhaomeng

zhuizhuhaomeng commented 1 year ago

@oowl I think we need to revert the previous PR and disable in GRPC only. cc @splitice

HTTP2 is stream base protocol, read the whole body is theoretically impossible, and it does not make sense.

Will a large HTTP request body in http2 connection block other HTTP requestes in the same connection? @oowl

splitice commented 1 year ago

It will block the same amount as http/1.1.That should be acceptable and consistent in behaviour.

oowl commented 1 year ago

Will a large HTTP request body in http2 connection block other HTTP requestes in the same connection? @oowl

No, Nginx process HTTP2 one ngx_http_request_t per stream, so every stream is isolated, one stream traffic spike can not influence other HTTP2 stream traffic.

And I have chat with @zhuizhuhaomeng on IM about https://github.com/openresty/lua-nginx-module/pull/2174 , It can not be revert, Openresty can not support HTTP2 read body now, So maybe we need to consider this is a feature(read _body in HTTP2)， And I think read_body timeout is a good idea in HTTP2 processing traffic. @zhuizhuhaomeng @splitice

it's also blocked in test-nginx https://github.com/openresty/test-nginx/blob/master/lib/Test/Nginx/Util.pm#L2910

swananan commented 1 year ago

Hi, HTTP/3 also faces similar issues in OpenResty. I have been thinking about how to address this problem. For achieving full-duplex communication in both HTTP/2 and HTTP/3, we can consider extending the functionalities of ngx.req.socket to enable handling the traffic interaction within a single stream in OpenResty's Lua module. This is because there are still several limitations when it comes to the read_body API, even with timeout support.

In my opinion, we could tackle this issue with a two-step solution:

Step 1: Revert the previous pull request and introduce support for a timeout option in read_body.

Step 2: Need to check whether ngx.req.socket works properly in the HTTP/2 or HTTP/3, if not, then enhance the capabilities of ngx.req.socket to accommodate both HTTP/2 and HTTP/3.

I am willing to take on these tasks to address this issue.

splitice commented 1 year ago

BTW I've reverted this for two different client applications and continue to experience no real problems. We don't have the concern of spiked requests though.

The problems really are limited to certain use cases, use cases that could do with a line in the documentation.

oowl commented 1 year ago

Hi, HTTP/3 also faces similar issues in OpenResty. I have been thinking about how to address this problem. For achieving full-duplex communication in both HTTP/2 and HTTP/3, we can consider extending the functionalities of ngx.req.socket to enable handling the traffic interaction within a single stream in OpenResty's Lua module. This is because there are still several limitations when it comes to the read_body API, even with timeout support.

In my opinion, we could tackle this issue with a two-step solution:

Step 1: Revert the previous pull request and introduce support for a timeout option in read_body.

Step 2: Need to check whether ngx.req.socket works properly in the HTTP/2 or HTTP/3, if not, then enhance the capabilities of ngx.req.socket to accommodate both HTTP/2 and HTTP/3.

I am willing to take on these tasks to address this issue.

I agree, So we need to know what is the openresty option. cc @zhuizhuhaomeng If you guys need any help, Please feel free to pin me. Or i can contribute something.

zhuizhuhaomeng commented 1 year ago

@splitice Would you please show the config of your usages?

splitice commented 1 year ago

What do you want @zhuizhuhaomeng?

dndx commented 6 months ago

We recently got affected by this change as well. IMO we should not add too much restrictions to the ngx.req.read_body() API for HTTP/2 as it severely limits the usefulness of this API call, client_body_timeout should already handles the timeout issue and we don't need to invent another param for such rare occurrence.

ngx.req.read_body() should be a thin wrapper around ngx_http_read_client_request_body() only, the potential for endless stream should and is already handled by client_max_body_size and client_body_timeout. We should not invent more limitations inside the OpenResty API but leave the freedom to the end user instead.

splitice commented 6 months ago

@dndx I 100% agree with your points.

I think the root of the actual problem is that client_body_timeout is between reads. Which the nginx folks think is OK (I agree) and the openresty folks do not. This is definately not the way it should be fixed.

For immediate effect fork and revert the commit. Thats what I'll be doing for the forseeable future.

dndx commented 6 months ago

I think the root of the actual problem is that client_body_timeout is between reads. Which the nginx folks think is OK (I agree) and the openresty folks do not.

This should not been an issue. Even with Content-Length provided inside the request, the user can send 1 byte every 60 seconds to not trigger the timeout for a long time. If this hasn't caused a problem in HTTP/1 then certainly it won't be an issue in HTTP/2 either. Why treat HTTP/2 differently?

zhuizhuhaomeng commented 6 months ago

If there is no problem getting the HTTP request body, then this restriction should be lifted. If there are some cases that will time out, then we add them to the document. Otherwise, this will break backward compatibility. @swananan

swananan commented 6 months ago

I think the root of the actual problem is that client_body_timeout is between reads. Which the nginx folks think is OK (I agree) and the openresty folks do not.

This should not been an issue. Even with Content-Length provided inside the request, the user can send 1 byte every 60 seconds to not trigger the timeout for a long time. If this hasn't caused a problem in HTTP/1 then certainly it won't be an issue in HTTP/2 either. Why treat HTTP/2 differently?

Hi, #2174 disabled the ngx.read_body API in HTTP/2 due to #2172, and #2237 tried to reduce the limitations, so #2237 allows the request with Content-Length to use the read_body api, I was putting all my focus into preventing #2172 from happening again. Currently, it doesn't seem like a good idea, as @dndx said, we should let the end user make their choice, especially since OpenResty already supported the ngx.read_body in HTTP/2.

@zhuizhuhaomeng So #2174 and #2237 should be reverted, and we need to enhance the doc about the #2172 scenario, and give suggestions like using HTTP/2 end stream flag and client_body_timeout.

oowl commented 6 months ago

I agree with revert https://github.com/openresty/lua-nginx-module/pull/2174, Sacrificing the majority of user usage for the sake of a handful of technically perfect solutions does not seem reasonable in some cases, apologies for this, I was thinking too much about the technical stuff before.

oowl commented 6 months ago

But I want to emphasize that client_body_timeout does not look like a solution, it would make ngx.read_body() return no content and directly let Nginx return a 408 status code without any logs. More like a last-resort strategy

oowl commented 6 months ago

Hi, I have submitted PR for the revert commit above these https://github.com/openresty/lua-nginx-module/pull/2286, pls help me review.

oowl commented 6 months ago

https://github.com/openresty/lua-nginx-module/commit/8dec675832574bdaf33e59258545b38633821137 https://github.com/openresty/lua-nginx-module/commit/6e29c1a96e641ea3e3498b1524baeaa28d3ab58c https://github.com/openresty/lua-nginx-module/commit/e0d19f787e74ce3ffbb9b9c5abad02fd6ea81f41

These PRs have been merged into the master branch, So can we close this issue? Sorry for causing trouble to everyone