search

openresty / lua-resty-limit-traffic

Lua library for limiting and controlling traffic in OpenResty/ngx_lua
819 stars 150 forks source link

fix: miss to uncommit the last one in resty.limit.traffic's list #54

Closed downtown12 closed 4 years ago

downtown12 commented 4 years ago

I fix a bug which may make conn limiter's shared.dict counters increase abnormally. This is because the original code snippets forget to uncommit the last limiter if a former one returns reject. Here is an example: we have a traffic limiter that combines two conn limiters:

{
    {'conn': 2, 'burst': 0, 'default_conn_delay': 0.1, 'key': 'binary_remote_addr'},
    {'conn': 10, 'burst': 0, 'default_conn_delay': 0.1, 'key': 'http_host'}
}

The first one will always reach reject status before the second one does. And the first one will end request(exiting 503 etc..), so the second conn limiter actually doesn't commit the request. but in the code, it's shared.dict value will raise by 1..And users may forget(or have no chance) to set the commited limiter into ngx.ctx. if ngx.ctx is unable to be set, leaving this request is impossible in log phase. So the connection value of the last limiter keeps increasing abnormally.

So I add an uncommit operation to the last index of the limiters, which I think is in accordance with the combine mehtod's logic: one should uncommit all of the other limiters if one limiter has already rejected request.

spacewander commented 4 years ago

LGTM. Can you provide a test case? We need a unit test to cover it.

downtown12 commented 4 years ago

LGTM. Can you provide a test case? We need a unit test to cover it.

Sure. I will provide it later.

downtown12 commented 4 years ago

LGTM. Can you provide a test case? We need a unit test to cover it.

Hi spacewander, I just add a test case (TEST 5) to cover my fix. The logic is almost the same as the TEST 4, but I add a output of the last limiter's counter to see what happens on it. Please check if it covers.

spacewander commented 4 years ago

@downtown12 LGTM. We need one more approval to merge the PR.

CC @doujiang24 @rainingmaster @zhuizhuhaomeng What are your opinions?

downtown12 commented 4 years ago

@downtown12 LGTM. We need one more approval to merge the PR.

CC @doujiang24 @rainingmaster @zhuizhuhaomeng What are your opinions?

hi @doujiang24 @rainingmaster @zhuizhuhaomeng, dear maintainers, please check if the PR is ok. And thanks.

doujiang24 commented 4 years ago

@downtown12 Nice catch, thanks for your contribution.