Hi Willy,
Le 19/08/2018 à 07:34, Willy Tarreau a écrit :
Hi Cyril,
Thanks for this, however there remains one (small) corner case : if dtmout
is equal to INT_MAX (2147483647), it will be rounded up by the +0.5 then
become negative :
Well, the original comment is not valid. The behaviour is not to round
up every values but to round values to the nearest int.
On Sun, Aug 19, 2018 at 12:27:13AM +0200, Cyril Bonté wrote:
if (dtmout > INT_MAX) /* overflow check */
- WILL_LJMP(luaL_error(L, "settimeout: cannot set values larger than
%d", INT_MAX));
+ WILL_LJMP(luaL_error(L, "settimeout: cannot set values larger than
%d ms", INT_MAX));
Above it will pass the test.
- tmout = MS_TO_TICKS((int)dtmout);
+ /* round up the timeout to convert it to int */
+ tmout = MS_TO_TICKS((int)(dtmout + 0.5));
and here we convert 2147483647.5 to int, which becomes -2147483648.
2147483647.5, when cast to int, should become 2147483647.
Given that the double to int conversion already rounds values, we do not
need the +0.5 so we can get rid of it. Alternatively, dtmout could be
rounded up before the checks using round() but apparently this is not
needed.
I suspect that the +0.5 might have been placed here to ensure that no
short timeout is rounded down to zero.
Adding 0.5 is a trick to round positive values to the nearest int (it
works in most cases). For example :
1234.1 will become 1234
1234.6 will become 1235
If that's the case we could
simply proceed like this :
if (dtmout < 0)
...
if (dtmout > INT_MAX)
...
tmout = MS_TO_TICKS((int)dtmout);
if (tmout == 0)
tmout++; // min 1ms
We're talking about loosing 1ms of precision, so the rounding trick is
maybe an overkill feature. I'm fine with removing the 0.5 addition and
adding 1ms if the result is equal to 0. And I clearly prefer that
behaviour ! Btw, this can already happen for very small values.
Currently, sock:settimeout(0.0001) will result in a 0ms timeout.
Just let me know if you want me to adjust the patch one way or another
so that you don't need to respin one.
I'm going to send you a new patch, which actually consists in including
what you've suggested ;-)
--
Cyril Bonté
