[tarantool-patches] Re: [PATCH] Fix lua socket polling in case of a spurious wakeup

[Vladislav Shpilevoy]

Thanks for the fixes!

Please, use [PATCH v2] prefix when sending second
version of a patch.

See 2 comments below.

On 18/07/2018 19:46, Georgy Kirichenko wrote:
> socket_writable/socket_readable may return before timeout is exceeded
> with the false status in cause of a spurious wakeup and this should not be
> treated as an EOF or an error.
> 
> Fixed #3344
> ---
> Issue: https://github.com/tarantool/tarantool/issues/3344
> Branch:
> https://github.com/tarantool/tarantool/tree/g.kirichenko/gh-3344-socket-io-spurios-wakeup
>   src/lua/socket.lua       | 57 ++++++++++++++++----------------
>   test/app/socket.result   | 70 ++++++++++++++++++++++++++++++++++++++++
>   test/app/socket.test.lua | 29 +++++++++++++++++
>   3 files changed, 129 insertions(+), 27 deletions(-)
> 
> diff --git a/src/lua/socket.lua b/src/lua/socket.lua
> index 06306eae2..6ebcea055 100644
> --- a/src/lua/socket.lua
> +++ b/src/lua/socket.lua
> @@ -337,11 +337,15 @@ local function do_wait(self, what, timeout)
>   end

1. The function 'do_wait' now sets _errno to timeout
even if it was actually not timeout but spurious wakeup.
I think, we should not set any errors in such a case.
Actually, do_wait is called from only three functions,
so we can set _errno in them when the timeout really
occurs.

>   
>   local function socket_readable(self, timeout)
> -    return do_wait(self, 1, timeout) ~= 0
> +    local result = do_wait(self, 1, timeout) ~= 0

2. Function socket_readable now can return even if the
socket actually is not readable because of spurious
wakeup. It can lead to block on read() as I understand,
if we are trying to read non-readable thing. And breaks
logic of this function.

I think, that here we should check do_wait result to
actually contain 'R'. If not 'R' and the timeout still
is not depleted, we should call do_wait again.

> +    fiber.testcancel()
> +    return result
>   end
>   
>   local function socket_writable(self, timeout)
> -    return do_wait(self, 2, timeout) ~= 0
> +    local result = do_wait(self, 2, timeout) ~= 0
> +    fiber.testcancel()
> +    return result
>   end
>   
>   local function socket_wait(self, timeout)