google / nginx-sxg-module

NGINX SXG module
Apache License 2.0
80 stars 18 forks source link

Don't preload more than 20 subresources #67

Open orisano opened 4 years ago

orisano commented 4 years ago

nginx's subrequest has a limit (NGX_HTTP_MAX_SUBREQUESTS=50).

https://github.com/nginx/nginx/blob/0a683fdd9313b9796bf39442fd117beaa63a7157/src/http/ngx_http_request.c#L628

https://github.com/nginx/nginx/blob/0a683fdd9313b9796bf39442fd117beaa63a7157/src/http/ngx_http_request.h#L13

What should ngx_http_sxg_filter_module behave when it exceeds the limit of nginx's subrequest?

https://github.com/google/nginx-sxg-module/blob/0ffe86cae9b1d54179327d70ac2c48e848a1f63d/ngx_http_sxg_filter_module.c#L533-L535

kumagi commented 4 years ago

Preloading too many subresources does not help speed up but harmful for network. So limiting the number is reasonable idea. The concern is rather a lack of care from webmaster. So appending some warning message in log and just remove extra preload headers would be enough. In current implementation, all preload header is eliminated but I'm not sure the behavior of touching the subrequests limit.

twifkak commented 3 years ago

I think we should omit rel=preload headers when the fetch fails, but not eliminate the successful ones.

(It's hypothetically possible for a page to depend on preload behavior, but I suspect rare and unadvised.)

Additionally, the Google SXG cache places a limit of 20 preloads. It may be worth providing an option to trim preloads to that limit.

twifkak commented 3 years ago

NGX_HTTP_MAX_SUBREQUESTS refers to the maximum subrequest nesting level, for deeply recursive subrequests. It is not the maximum number of independent subrequests to make. I successfully tested an example with 52 preloads.

Nonetheless, we should set a limit for the sake of the Google SXG Cache requirement.

orisano commented 3 years ago

Sorry, I misunderstood it.

twifkak commented 3 years ago

No worries; I am just beginning to understand NGINX myself.