From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 6256A46970E
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 26 Dec 2019 02:43:30 +0300 (MSK)
Date: Thu, 26 Dec 2019 02:43:27 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20191225234327.hfecptmxityb7fko@tkn_work_nb>
References: <20191225210407.32120-1-i.kosarev@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20191225210407.32120-1-i.kosarev@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v5] test: fix flaky socket test
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Ilya Kosarev <i.kosarev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org

Now it works like a charm with 30 jobs and mostly works with 50 jobs in
parallel on my machine. This is the good change.

A few notes below.

WBR, Alexander Turenko.

> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4426-4451-fix-socket-test
> Issues: https://github.com/tarantool/tarantool/issues/4426
>         https://github.com/tarantool/tarantool/issues/4451
>         https://github.com/tarantool/tarantool/issues/4469

> @@ -230,7 +230,7 @@ function aexitst(ai, hostnames, port)
>  end;
>  
>  
> -aexitst( socket.getaddrinfo('localhost', 'http', {  protocol = 'tcp',
> +aexitst( socket.getaddrinfo('127.0.0.1', 'http', {  protocol = 'tcp',
>      type = 'SOCK_STREAM'}), {'127.0.0.1', '::1'}, 80 );

This is the test case for getaddrinfo() itself. It is better to check
here that 'localhost' actually resolved to either 127.0.0.1 or ::1
(typically both, but it depends of IPv4 / IPv6 support on a particular
system).

> @@ -546,7 +535,7 @@ socket.getaddrinfo('host', 'port', { flags = 'WRONG' }) == nil and errno() == er
>  test_run:cmd("setopt delimiter ';'")
>  f = fiber.create(function()
>      while true do
> -        local result = socket.getaddrinfo('localhost', '80')
> +        local result = socket.getaddrinfo('127.0.0.1', '80')
>          fiber.sleep(0)
>      end
>  end);

Same here.

> @@ -955,17 +964,17 @@ client:write('world')
>  client:read(5, 5) == 'world'
>  -- cancel fiber
>  fiber.cancel(echo_fiber)
> -client:read(1, 5) == ''
> +client:read(1, TIMEOUT) == ''
>  server:close()

It worth to use TIMEOUT constant instead of 5 in two near :read() calls
above this one:

 | client:read(5, 5) == 'hello'
 | client:read(5, 5) == 'world'

There is also other case where it is applicable:

 | sc:send('Hello')
 | sc:send(', world')
 | sc:send("\\nnew line")
 | sa:read('\\n', 1)                           -- !!
 | sa:read({ chunk = 1, delimiter = 'ine'}, 1) -- !!
 | sa:read('ine', 1)                           -- !!
 | sa:read('ine', 0.1)

All reads except the last one should use the common TIMEOUT. The last
one expected to block on 0.1 seconds and then return nil, so don't
increase timeout here.

And the case right after this one:

 | sc:send('Hello, world')
 | sa:read(',', 1)                             -- !!
 | sc:shutdown('W')
 | sa:close()
 | sc:close()

This way the change will be consistent within the file, at least in
context of :read() calls with a timeout.

Those cases do not fail for me (I don't know why), so it is purely for
consistency. Existing test cases will show our practices, that's good
for ones who'll write more cases in a future.