Mark Lakes
lua socket api settimeout in seconds vs. milliseconds
March 07, 2018 06:20PM
In regards to earlier conversation, herein is a patch attached for the
feature.
From the mail archive:
https://www.mail-archive.com/[email protected]/msg27806.html
https://www.mail-archive.com/[email protected]/msg27807.html

Mark Lakes

Signal Sciences | www.signalsciences.com |


conversation participants:
Willy Tarreau
Adis Nezirovic
Nick Galbreath

--------- Last conversation and decision agreement ----------

Nick Galbreath
https://www.mail-archive.com/[email protected]&q=from:%22Nick+Galbreath%22
Thu,
09 Nov 2017 20:44:28 -0800
https://www.mail-archive.com/[email protected]&q=date:20171109

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 <[email protected]> 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
>


----------
Attachments:
open | download - 0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch (2.2 KB)
Thierry Fournier
Re: lua socket api settimeout in seconds vs. milliseconds
March 07, 2018 07:00PM
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 <[email protected]> wrote:
>
> In regards to earlier conversation, herein is a patch attached for the feature.
> From the mail archive:
> https://www.mail-archive.com/[email protected]/msg27806.html
> https://www.mail-archive.com/[email protected]/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 <[email protected]
> > 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>
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 <[email protected]>
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 <[email protected]> wrote:
> >
> > In regards to earlier conversation, herein is a patch attached for the
> feature.
> > From the mail archive:
> > https://www.mail-archive.com/[email protected]/msg27806.html
> > https://www.mail-archive.com/[email protected]/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 <[email protected]
> > > 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>
>
>
Attachments:
open | download - 0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch (1.4 KB)
Thierry Fournier
Re: lua socket api settimeout in seconds vs. milliseconds
March 08, 2018 10:30AM
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






> On 8 Mar 2018, at 01:17, Mark Lakes <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >
> > In regards to earlier conversation, herein is a patch attached for the feature.
> > From the mail archive:
> > https://www.mail-archive.com/[email protected]/msg27806.html
> > https://www.mail-archive.com/[email protected]/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 <[email protected]
> > > 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>
Attachments:
open | download - 0001-BUG-MINOR-lua-the-function-returns-anything.patch (1012 bytes)
open | download - 0002-BUG-MINOR-lua-funtion-hlua_socket_settimeout-don-t-c.patch (1.1 KB)
open | download - 0003-MINOR-lua-typo-fix.patch (1.1 KB)
Tim Düsterhus
Re: lua socket api settimeout in seconds vs. milliseconds
March 08, 2018 03:10PM
Thierry,

Am 08.03.2018 um 10:24 schrieb Thierry Fournier:
> 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:
>

Your first patch will be regressing my commit
119a5f10e47f3507e58116002583e1226473485d, which specifically changed the
return value to be 1 for compatibility with the standard Socket class of
Lua!

http://git.haproxy.org/?p=haproxy.git;a=commit;h=119a5f10e47f3507e58116002583e1226473485d

Best regards
Tim Düsterhus
Thierry Fournier
Re: lua socket api settimeout in seconds vs. milliseconds
March 08, 2018 03:20PM
> On 8 Mar 2018, at 15:03, Tim Düsterhus <[email protected]> wrote:
>
> Thierry,
>
> Am 08.03.2018 um 10:24 schrieb Thierry Fournier:
>> 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:
>>
>
> Your first patch will be regressing my commit
> 119a5f10e47f3507e58116002583e1226473485d, which specifically changed the
> return value to be 1 for compatibility with the standard Socket class of
> Lua!
>
> http://git.haproxy.org/?p=haproxy.git;a=commit;h=119a5f10e47f3507e58116002583e1226473485d


Ok, Lua expect the number of elements ins the stack. The right way for returning 1 is:

lua_pushinteger(L, 1);
return 1;

I will add this in the patch.

BR,
Thierry
Tim Düsterhus
Re: lua socket api settimeout in seconds vs. milliseconds
March 08, 2018 03:20PM
Hi

Am 08.03.2018 um 15:10 schrieb Thierry Fournier:
> Ok, Lua expect the number of elements ins the stack. The right way for returning 1 is:
>
> lua_pushinteger(L, 1);
> return 1;
>

Okay, then my patch probably worked, because of whatever value was left
on the stack. I learned something new here!

Indeed the original socket class pushes `1` to the stack (but it uses
lua_pushnumber, instead of pushinteger). For the reference here's the
original line of code:
https://github.com/diegonehab/luasocket/blob/316a9455b9cb4637fe6e62b20fbe05f5141fec54/src/timeout.c#L172

Best regards
Tim Düsterhus
Thierry Fournier
Re: lua socket api settimeout in seconds vs. milliseconds
March 08, 2018 09:20PM
> On 8 Mar 2018, at 15:14, Tim Düsterhus <[email protected]> wrote:
>
> Hi
>
> Am 08.03.2018 um 15:10 schrieb Thierry Fournier:
>> Ok, Lua expect the number of elements ins the stack. The right way for returning 1 is:
>>
>> lua_pushinteger(L, 1);
>> return 1;
>>
>
> Okay, then my patch probably worked, because of whatever value was left
> on the stack. I learned something new here!
>
> Indeed the original socket class pushes `1` to the stack (but it uses
> lua_pushnumber, instead of pushinteger). For the reference here's the
> original line of code:
> https://github.com/diegonehab/luasocket/blob/316a9455b9cb4637fe6e62b20fbe05f5141fec54/src/timeout.c#L172


Thanks for the search int the LuaSocket code. The doc doesn’t tell
anything about the returned value.

I think that the original patch worked because the top of the stack
was the timeout value given as argument.

Hey, the example of use of socket.http in attachment of your original
commit is great !

3 new patch in attachement to consider for the initial subject of
this thread.

BR,
Thierry
Attachments:
open | download - 0001-BUG-MINOR-lua-the-function-returns-anything.patch (1.2 KB)
open | download - 0002-BUG-MINOR-lua-funtion-hlua_socket_settimeout-don-t-c.patch (1.1 KB)
open | download - 0003-MINOR-lua-typo-fix.patch (1.1 KB)
Tim Düsterhus
Re: lua socket api settimeout in seconds vs. milliseconds
March 08, 2018 10:50PM
Thierry,

Am 08.03.2018 um 21:15 schrieb Thierry Fournier:
> Hey, the example of use of socket.http in attachment of your original
> commit is great !

If you are curious what I built with that and in case you missed my list
mail advertising the project:
https://github.com/TimWolla/haproxy-auth-request

> 3 new patch in attachement to consider for the initial subject of
> this thread.

I took a look at it:

> The LuaSocket documentation tell anything about the returned value,
> but the effective code set an integer of value one.

Should this read: "The LuaSocket documentation __does not__ tell
anything about the returned value ..."?

There's a few more regular typos (spelling of words) in the commit
messages, but that specific part is misleadingly worded, thus I wanted
to make you aware of it.

Best regards
Tim Düsterhus
Thierry Fournier
Re: lua socket api settimeout in seconds vs. milliseconds
March 09, 2018 10:30AM
> On 8 Mar 2018, at 22:41, Tim Düsterhus <[email protected]> wrote:
>
> Thierry,
>
> Am 08.03.2018 um 21:15 schrieb Thierry Fournier:
>> Hey, the example of use of socket.http in attachment of your original
>> commit is great !
>
> If you are curious what I built with that and in case you missed my list
> mail advertising the project:
> https://github.com/TimWolla/haproxy-auth-request
>


Interesting. I add a link on my blog: http://blog.arpalert.org/p/interesting-links.html


>> 3 new patch in attachement to consider for the initial subject of
>> this thread.
>
> I took a look at it:
>
>> The LuaSocket documentation tell anything about the returned value,
>> but the effective code set an integer of value one.
>
> Should this read: "The LuaSocket documentation __does not__ tell
> anything about the returned value …"?


Yes. I dont see anything about the returned value.


> There's a few more regular typos (spelling of words) in the commit
> messages, but that specific part is misleadingly worded, thus I wanted
> to make you aware of it.


ok, thanks.

BR,
Thierry
Tim Düsterhus
Re: lua socket api settimeout in seconds vs. milliseconds
March 26, 2018 12:20AM
Willy,

Am 08.03.2018 um 21:15 schrieb Thierry Fournier:
> 3 new patch in attachement to consider for the initial subject of
> this thread.
>

did you miss these patches from Thierry to the Lua subsystem?

Best regards
Tim Düsterhus
Willy Tarreau
Re: lua socket api settimeout in seconds vs. milliseconds
March 26, 2018 12:50AM
Hi Tim,

On Mon, Mar 26, 2018 at 12:15:02AM +0200, Tim Düsterhus wrote:
> Willy,
>
> Am 08.03.2018 um 21:15 schrieb Thierry Fournier:
> > 3 new patch in attachement to consider for the initial subject of
> > this thread.
> >
>
> did you miss these patches from Thierry to the Lua subsystem?

Hmmm seems so, I'll recheck tomorrow. Thanks.

Willy
Willy Tarreau
Re: lua socket api settimeout in seconds vs. milliseconds
March 26, 2018 11:20AM
On Mon, Mar 26, 2018 at 12:40:19AM +0200, Willy Tarreau wrote:
> > did you miss these patches from Thierry to the Lua subsystem?
>
> Hmmm seems so, I'll recheck tomorrow. Thanks.

Now applied. Thanks for notifying!
Willy
Hi Thierry, remade patch for support of number other than integer in
hlua_socket_settimeout() ...from latest source so should be able to be
applied.

Desc: instead of accepting only integers, allow user to specify float and
double when hlua_socket_settimeout() is called. Round up user specified
value and validate against maximum integer to prevent overflow.

Mark Lakes
Signal Sciences | www.signalsciences.com |


On Thu, Mar 8, 2018 at 1:24 AM, Thierry Fournier <[email protected]>
wrote:

> 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
>
>
>
>
>
>
>
> > On 8 Mar 2018, at 01:17, Mark Lakes <[email protected]> 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 <[email protected]>
> 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 <[email protected]> wrote:
> > >
> > > In regards to earlier conversation, herein is a patch attached for the
> feature.
> > > From the mail archive:
> > > https://www.mail-archive.com/[email protected]/msg27806.html
> > > https://www.mail-archive.com/[email protected]/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 <[email protected]
> > > > 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>
>
>
>
Attachments:
open | download - 0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch (2 KB)
Thierry Fournier
Re: lua socket api settimeout in seconds vs. milliseconds
March 27, 2018 10:00AM
Thank Mark,

Willy, could you apply the attached patch.

For information, I updated doc and add comment in the commit message.

Thierry


> On 27 Mar 2018, at 06:49, Mark Lakes <[email protected]> wrote:
>
> Hi Thierry, remade patch for support of number other than integer in hlua_socket_settimeout() ...from latest source so should be able to be applied.
>
> Desc: instead of accepting only integers, allow user to specify float and double when hlua_socket_settimeout() is called. Round up user specified value and validate against maximum integer to prevent overflow.
>
> Mark Lakes
> Signal Sciences | www.signalsciences.com |
>
>
> On Thu, Mar 8, 2018 at 1:24 AM, Thierry Fournier <[email protected]> wrote:
> 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
>
>
>
>
>
>
>
> > On 8 Mar 2018, at 01:17, Mark Lakes <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > >
> > > In regards to earlier conversation, herein is a patch attached for the feature.
> > > From the mail archive:
> > > https://www.mail-archive.com/[email protected]/msg27806.html
> > > https://www.mail-archive.com/[email protected]/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 <[email protected]
> > > > 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>
>
>
>
> <0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch>
Attachments:
open | download - 0001-MINOR-lua-allow-socket-api-settimeout-to-accept-inte.patch (3.1 KB)
Willy Tarreau
Re: lua socket api settimeout in seconds vs. milliseconds
March 27, 2018 02:30PM
On Tue, Mar 27, 2018 at 09:51:51AM +0200, Thierry Fournier wrote:
> Thank Mark,
>
> Willy, could you apply the attached patch.
>
> For information, I updated doc and add comment in the commit message.

Applied, thanks guys!
willy
Sorry, only registered users may post in this forum.

Click here to login