Hi Mark, Thanks, it seems perfect. But, I can’t apply on current master branch, the patch is rejected.
I forgot 3 things while my first read: - The Lua error are not trigerred with a return 1 (the return 1 is a bug in the original function), but with something like that: WILL_LJMP(luaL_error(L, "settimeout: cannot set negatives values")); The return became useless because the luaL_error() function never returns. (it soes a longjmp call). - I think that timeouts < 1s are allowed. The cli refuse these ones, but the HAProxy core is ready to work with timeout less than 1s. Note that, When you remove this check, negative timeouts could be accepted (the check < 1000 chek also for negative values) and this is a bug. - The doc (doc/lua-api/index.rst) should be updated (actuelly, line 1899: .. js:function:: Socket.settimeout(socket, value [, mode])). It will be necessary to precise that the fucntion accept Number in place of Integer and, number with fractional part are allowed. I join two of my fixes. The third patch is yours, check if you agree with the content and the commit message. BR, Thierry
0001-BUG-MINOR-lua-the-function-returns-anything.patch
Description: Binary data
0002-BUG-MINOR-lua-funtion-hlua_socket_settimeout-don-t-c.patch
Description: Binary data
0003-MINOR-lua-typo-fix.patch
Description: Binary data
> On 8 Mar 2018, at 01:17, Mark Lakes <mla...@signalsciences.com> wrote: > > Hi Thierry, thanks for feedback. Addressed concerns in the new attached patch. > > http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout > > Description: instead of hlua_socket_settimeout() accepting only integers, > allow user to specify float and > double as well. Convert to milliseconds much like cli_parse_set_timeout but > also sanity check the value. > > -mark > > > On Wed, Mar 7, 2018 at 9:55 AM, Thierry Fournier <tfourn...@arpalert.org> > wrote: > Hi Mark, > > Thanks for the patch. I don’t like usage of floating point, but the > luasocket documentation says that the settimeout() function accept only > second. In this case, the usage of floating point seems be to be a good > way. > > Can you split in a second commit the fix of comments from the effective > patch, and avoid this kind of changes: > > - int tmout; > + int tmout; > > Just because, this kind of changes are useless, and it add noisy > information in the patch. > > A last point: could you explain int the message of the patch the > goal of these patch. To avoid a search, this is the link of the official > luasocket setimeout function: > > http://w3.impa.br/~diego/software/luasocket/tcp.html#settimeout > > Thanks > Thierry > > > > On 7 Mar 2018, at 18:16, Mark Lakes <mla...@signalsciences.com> wrote: > > > > In regards to earlier conversation, herein is a patch attached for the > > feature. > > From the mail archive: > > https://www.mail-archive.com/haproxy@formilux.org/msg27806.html > > https://www.mail-archive.com/haproxy@formilux.org/msg27807.html > > > > Mark Lakes > > Signal Sciences | www.signalsciences.com | > > > > conversation participants: > > Willy Tarreau > > Adis Nezirovic > > Nick Galbreath > > > > --------- Last conversation and decision agreement ---------- > > Nick Galbreath Thu, 09 Nov 2017 20:44:28 -0800 > > > > thanks wily. > > > > re: " CONTRIBUTING in the sources directory," - > > > > yes, that is what I was looking for! thanks for the tip. > > > > re: least it seems important to round up non-null values to the next > > millisecond. > > > > Definitely, we can and should add some checks for invalid values, etc. > > > > I'll read CONTRIBUTING, and set up my dev env, try a patch, and report > > back appropriately. > > > > regards, > > > > n > > > > On Thu, Nov 9, 2017 at 8:37 PM, Willy Tarreau <w...@1wt.eu > > > wrote: > > > > > Hi Nick, > > > > > > On Thu, Nov 09, 2017 at 08:27:29PM -0800, Nick Galbreath wrote: > > > > Hello Adis, > > > > > > > > We could certainly add another API/Lua function but it might be easier > > > > to > > > > change > > > > > > > > luaL_checkinteger(L, 2) in > > > > > > > > tmout = MAY_LJMP(luaL_checkinteger(L, 2)) * 1000; > > > > > > > > to luaL_checknumber(L, 2), along with appropriate cast to int. > > > > > > > > Then we have backwards compatibility, less documentation to write, and > > > get > > > > millisecond timeouts. > > > > > > At least it seems important to round up non-null values to the next > > > millisecond, otherwise we may observe busy loops when users specify > > > sleep delays smaller than the millisecond, as haproxy's internal > > > clock is millisecond-based (poll()'s resolution). > > > > > > > > > If people want a separate API, I'm happy to do that too, just more work. > > > > > > I think it should work as you propose it, more or less the round up of > > > course. > > > > > > > Please advise, and I'll make a patch either way. I'm unfamiliar with > > > > the > > > > HAProxy development process, so any tips or pointers are welcome, > > > > > > It's important to CC the subsystem maintainer when submitting a change, > > > since they are supposed to have the last word on submissions in their > > > area. This is done here since Thierry maintains the Lua area. Please > > > carefully read CONTRIBUTING in the sources directory, it's not very > > > long and will help you ensure that all your patches are easily merged. > > > And you're welcome to propose changes to this file if something is > > > unclear :-) > > > > > > Thanks, > > > Willy > > > > > > > > > ---------- > > > > > > > > <0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch> > > > <0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch>
