[PATCH] Allow using nodelay=N semantics in limit_req configuration

[Maxim Dounin]

Hello!

On Mon, Oct 29, 2018 at 05:14:13PM +0300, Щучкин Петр wrote:

>    2016 year patch seems pretty same to me  :) Interesting that I also
>    made my patch at 2016, but we have started to use it in production a
>    while ago (when upgraded to nginx 1.15.4 where you've fixed limit_req
>    issue)
> 
>    I think calculating excess above nodelay should be more expected
>    behavior. Think about setting large burst and large nodelay, for ex.
>    burst=10000 nodelay=9999. Even at relatively fast limit rate (10r/s)
>    this will therefore lead to very large delay which I think is not
>    expected nor by user neither by server administrator. I think it is
>    more intuitive to think of it as you give nodelay requests for for free
>    and additional requests with limited speed, instead of you give nodelay
>    requests as a credit and then force client to wait all that "credited"
>    time when he wants to make additional requests.

Yes, I agree.  As previously said, only taking into account the 
excess above the nodelay level looks more logical.

>    Also I  think it doesn't have sense to allow nodelay to be larger than
>    burst because in any case requests that are more then burst will be
>    rejected. May be it's better to just warn user and implicitly reduce
>    nodelay value to burst value in that case instead of error.

There is no difference between using the nodelay level as is or 
reducing it to burst level.  Nodelay level above the burst limit 
means that all requests will not be delayed - they are either 
allowed, or rejected due to burst limit.  Moreover, the default 
value as in my patch actualy uses very large nodelay value to 
implement "nodelay" without an explicitly specified value.

While limiting users from inadvertently using nodelay= larger than 
burst= may help to diagnose some configuration issues, I don't 
think it worth the effort.

>    So I would suggest to use behavior that calculates delay based on
>    excess above nodelay.

Here is an updated patch, which calculates delay based on the 
specified level.  Also, the parameter was renamed to "delay=", as 
this name looks more logical.

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1540913077 -10800
#      Tue Oct 30 18:24:37 2018 +0300
# Node ID 9697dadddc63e793501152b61c08519edab72f0f
# Parent  874d47ac871a4b62fbe0ff5d469a8ad7bc5a4160
Limit req: "delay=" parameter.

This parameter specifies an additional "soft" burst limit at which requests
become delayed (but not yet rejected as it happens if "burst=" limit is
exceeded).  Defaults to 0, i.e., all excess requests are delayed.

Originally inspired by Vladislav Shabanov
(http://mailman.nginx.org/pipermail/nginx-devel/2016-April/008126.html).
Further improved based on a patch by Peter Shchuchkin
(http://mailman.nginx.org/pipermail/nginx-devel/2018-October/011522.html).

diff --git a/src/http/modules/ngx_http_limit_req_module.c b/src/http/modules/ngx_http_limit_req_module.c
--- a/src/http/modules/ngx_http_limit_req_module.c
+++ b/src/http/modules/ngx_http_limit_req_module.c
@@ -44,7 +44,7 @@ typedef struct {
     ngx_shm_zone_t              *shm_zone;
     /* integer value, 1 corresponds to 0.001 r/s */
     ngx_uint_t                   burst;
-    ngx_uint_t                   nodelay; /* unsigned  nodelay:1 */
+    ngx_uint_t                   delay;
 } ngx_http_limit_req_limit_t;
 
 
@@ -499,12 +499,12 @@ ngx_http_limit_req_account(ngx_http_limi
 
     excess = *ep;
 
-    if (excess == 0 || (*limit)->nodelay) {
+    if ((ngx_uint_t) excess <= (*limit)->delay) {
         max_delay = 0;
 
     } else {
         ctx = (*limit)->shm_zone->data;
-        max_delay = excess * 1000 / ctx->rate;
+        max_delay = (excess - (*limit)->delay) * 1000 / ctx->rate;
     }
 
     while (n--) {
@@ -544,11 +544,11 @@ ngx_http_limit_req_account(ngx_http_limi
 
         ctx->node = NULL;
 
-        if (limits[n].nodelay) {
+        if ((ngx_uint_t) excess <= limits[n].delay) {
             continue;
         }
 
-        delay = excess * 1000 / ctx->rate;
+        delay = (excess - limits[n].delay) * 1000 / ctx->rate;
 
         if (delay > max_delay) {
             max_delay = delay;
@@ -875,9 +875,9 @@ ngx_http_limit_req(ngx_conf_t *cf, ngx_c
 {
     ngx_http_limit_req_conf_t  *lrcf = conf;
 
-    ngx_int_t                    burst;
+    ngx_int_t                    burst, delay;
     ngx_str_t                   *value, s;
-    ngx_uint_t                   i, nodelay;
+    ngx_uint_t                   i;
     ngx_shm_zone_t              *shm_zone;
     ngx_http_limit_req_limit_t  *limit, *limits;
 
@@ -885,7 +885,7 @@ ngx_http_limit_req(ngx_conf_t *cf, ngx_c
 
     shm_zone = NULL;
     burst = 0;
-    nodelay = 0;
+    delay = 0;
 
     for (i = 1; i < cf->args->nelts; i++) {
 
@@ -915,8 +915,20 @@ ngx_http_limit_req(ngx_conf_t *cf, ngx_c
             continue;
         }
 
+        if (ngx_strncmp(value[i].data, "delay=", 6) == 0) {
+
+            delay = ngx_atoi(value[i].data + 6, value[i].len - 6);
+            if (delay <= 0) {
+                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+                                   "invalid delay value \"%V\"", &value[i]);
+                return NGX_CONF_ERROR;
+            }
+
+            continue;
+        }
+
         if (ngx_strcmp(value[i].data, "nodelay") == 0) {
-            nodelay = 1;
+            delay = NGX_MAX_INT_T_VALUE / 1000;
             continue;
         }
 
@@ -956,7 +968,7 @@ ngx_http_limit_req(ngx_conf_t *cf, ngx_c
 
     limit->shm_zone = shm_zone;
     limit->burst = burst * 1000;
-    limit->nodelay = nodelay;
+    limit->delay = delay * 1000;
 
     return NGX_CONF_OK;
 }

-- 
Maxim Dounin
http://mdounin.ru/