lua-nginx-module icon indicating copy to clipboard operation
lua-nginx-module copied to clipboard

Published 20 hours ago • verified publisher icon openresty

bugfix: sanity checking in ngx.timer.at delay param

Open p0pr0ck5 opened this issue 7 years ago • 7 comments
trafficstars

I hereby granted the copyright of the changes in this pull request to the authors of this lua-nginx-module project.

Fixes #1278.

p0pr0ck5 avatar Mar 26 '18 20:03 p0pr0ck5

Just a side note: I wonder if we should omit all the API checks by macros like LuaJIT's LUA_USE_APICHECK macro. All such checks come with a cost and the accumulated overhead can be big if these API functions are called frequently.

agentzh avatar Mar 26 '18 23:03 agentzh

If there were detailed benchmarks showing the impact of such sanity checks, I would totally be onboard. I would hope that a few small branches here (very highly biased and well predicted) wouldn't cause a noticeable impact in the grand scheme of things.

p0pr0ck5 avatar Mar 27 '18 00:03 p0pr0ck5

@agentzh any thoughts here? I did some benchmarking with a macro like LUA_USE_APICHECK against this function and a few others (like ngx_http_lua_shdict_get_helper, ngx_http_lua_ngx_sleep, etc) and found no appreciable difference. Let me know if you want me to share my data or how you want to proceed on this PR.

p0pr0ck5 avatar Apr 17 '18 00:04 p0pr0ck5

This particular change is probably fine to be enabled by default as the cost of comparison is practically negligible compared to the cost of creating a timer.

dndx avatar Apr 20 '18 19:04 dndx

@thibaultcha @agentzh ping?

p0pr0ck5 avatar May 19 '19 03:05 p0pr0ck5

The trend is to check integer overflow for all API functions? I'm not sure if we want to do this everywhere. It's too much a burden IMHO.

agentzh avatar May 20 '19 19:05 agentzh

This pull request is now in conflict :(

mergify[bot] avatar Jun 26 '20 00:06 mergify[bot]