ghedo / http2-push-nginx-module

*DEPRECATED* NGINX now supports HTTP/2 server push out of the box http://hg.nginx.org/nginx/rev/641306096f5b
BSD 2-Clause "Simplified" License
40 stars 5 forks source link

Honor http2_max_pushed_streams setting #3

Closed StephanDollberg closed 7 years ago

StephanDollberg commented 7 years ago

Don't push more than http2_max_pushed_streams resources. Setting default to arbitrary value of 100.

StephanDollberg commented 7 years ago

Will fix the docs.

In regards to the check, yeah I am undecided on that.

Problem with checking for the length first is that if you were to push a resource which you already pushed, then it returns an error even though there is no real problem. (shrug)

On May 26, 2017 16:07, "Alessandro Ghedini" notifications@github.com wrote:

@ghedo requested changes on this pull request.

Looks good, just a few minor things.

In README.rst https://github.com/ghedo/http2-push-nginx-module/pull/3#discussion_r118722756 :

@@ -32,6 +32,15 @@ http2_push_path

Specifies a resource that should be pushed.

+http2_max_pushed_streams +~~~ + +syntax: http2_max_pushed_streams +

Should document default value (see e.g. http2_server_push docs).

In src/ngx_http_v2_push_module.c https://github.com/ghedo/http2-push-nginx-module/pull/3#discussion_r118723069 :

@@ -731,6 +746,10 @@ ngx_http_v2_push(ngx_http_request_t r, u_char u_str, size_t u_len) return NGX_OK; }

Probably best to put this before the other check since it's less expensive. Not too important though.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ghedo/http2-push-nginx-module/pull/3#pullrequestreview-40544657, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxSk5mRlH4xItEs-Rk7f_YnQgUbYHlfks5r9urEgaJpZM4NnsYr .

ghedo commented 7 years ago

Merged in 031acc46fd84907002fe2bf9bc7879e3c9d85fea, thanks!