[Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Alexander V. Tikhonov]

From: Alexander Turenko <alexander.turenko@tarantool.org>

Found issue running test on FreeBSD VBox host:

 [011] --- box/net.box_wait_connected_gh-3856.result	Mon Jun 15 09:39:49 2020
 [011] +++ box/net.box_wait_connected_gh-3856.reject	Fri May  8 08:23:30 2020
 [011] @@ -12,7 +12,8 @@
 [011]  - opts:
 [011]      wait_connected: false
 [011]    host: 8.8.8.8
 [011] -  state: initial
 [011] +  state: error
 [011] +  error: Invalid argument
 [011]    port: '123456'
 [011]  ...
 [011]  c:close()

The reason of the fail was that getaddrinfo() returned EIA_SERVICE for an
incorrect TCP/IP port on FreeBSD, but crops it as modulo of 65536 on
Linux/glibc. Checked with local script './getaddrinfo':

  (Linux/glibc) $ ./getaddrinfo 8.8.8.8 123456
  ----
  family: AF_INET
  socktype: SOCK_STREAM
  protocol: IPPROTO_TCP
  host: 8.8.8.8
  serv: 57920

  (FreeBSD) $ ./getaddrinfo 8.8.8.8 123456
  getaddrinfo: Service was not recognized for socket type

So obvious fix is to change 123456 to something less or equal to
65535. Say, 1234.

The test depended on an order in which fibers were scheduled
(net_box.connect() creates a separate fiber for connecting in background
using fiber.create(), which yields). Unlikely our fiber were not get
execution time during the connection attempt, so it was more like a
formal thing.

But we can decrease probability of this situation even more if we'll
grab all connection fields just when net_box.connect() returns, not
after yield in console (which is due to waiting a next command from
test-run).

Closes #5083

Reviewed-by: Alexander V. Tikhonov <avtikhon@tarantool.org>
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-5083-net-box-google-dns
Issue: https://github.com/tarantool/tarantool/issues/5083

 test/box/net.box_wait_connected_gh-3856.result   | 13 ++++++++-----
 test/box/net.box_wait_connected_gh-3856.test.lua |  8 ++++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/test/box/net.box_wait_connected_gh-3856.result b/test/box/net.box_wait_connected_gh-3856.result
index 9234e6cb9..0ac6d8a0b 100644
--- a/test/box/net.box_wait_connected_gh-3856.result
+++ b/test/box/net.box_wait_connected_gh-3856.result
@@ -1,19 +1,22 @@
 net = require('net.box')
 ---
 ...
+test_run = require('test_run').new()
+---
+...
 --
 -- gh-3856: wait_connected = false is ignored.
 --
-c = net.connect('8.8.8.8:123456', {wait_connected = false})
----
-...
-c
+do                                                            \
+    c = net.connect('8.8.8.8:1234', {wait_connected = false}) \
+    return c                                                  \
+end
 ---
 - opts:
     wait_connected: false
   host: 8.8.8.8
   state: initial
-  port: '123456'
+  port: '1234'
 ...
 c:close()
 ---
diff --git a/test/box/net.box_wait_connected_gh-3856.test.lua b/test/box/net.box_wait_connected_gh-3856.test.lua
index 29e997fb5..cf3974e98 100644
--- a/test/box/net.box_wait_connected_gh-3856.test.lua
+++ b/test/box/net.box_wait_connected_gh-3856.test.lua
@@ -1,8 +1,12 @@
 net = require('net.box')
+test_run = require('test_run').new()
 
 --
 -- gh-3856: wait_connected = false is ignored.
 --
-c = net.connect('8.8.8.8:123456', {wait_connected = false})
-c
+do                                                            \
+    c = net.connect('8.8.8.8:1234', {wait_connected = false}) \
+    return c                                                  \
+end
+
 c:close()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/test/box/net.box_wait_connected_gh-3856.result b/test/box/net.box_wait_connected_gh-3856.result
> index 9234e6cb9..0ac6d8a0b 100644
> --- a/test/box/net.box_wait_connected_gh-3856.result
> +++ b/test/box/net.box_wait_connected_gh-3856.result
> @@ -1,19 +1,22 @@
>  net = require('net.box')
>  ---
>  ...
> +test_run = require('test_run').new()
> +---
> +...
>  --
>  -- gh-3856: wait_connected = false is ignored.
>  --
> -c = net.connect('8.8.8.8:123456', {wait_connected = false})
> ----
> -...
> -c
> +do                                                            \
> +    c = net.connect('8.8.8.8:1234', {wait_connected = false}) \
> +    return c                                                  \

You said you grab all connection fields, but you don't. This
construction is 100% the same as

    c = net.connect('8.8.8.8:1234', {wait_connected = false})

Without 'do', 'return', and 'end'. In the original fix from
Alexander Tu. I see that he returned c.state, not just c.
But since you may need to call close() anyway, you probably
should return

    c, c.state

So as to print the second value and close the first value.
Another option - wrap all that into a function. Which calls
close() inside, and returns the state.

> +end
>  ---
>  - opts:
>      wait_connected: false
>    host: 8.8.8.8
>    state: initial
> -  port: '123456'
> +  port: '1234'
>  ...
>  c:close()

Re: [Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Alexander V. Tikhonov]

Hi Vlad, thanks for your review, I've corrected the test as you
suggested, please check my comments below.

On Sun, Jun 21, 2020 at 05:50:10PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/test/box/net.box_wait_connected_gh-3856.result b/test/box/net.box_wait_connected_gh-3856.result
> > index 9234e6cb9..0ac6d8a0b 100644
> > --- a/test/box/net.box_wait_connected_gh-3856.result
> > +++ b/test/box/net.box_wait_connected_gh-3856.result
> > @@ -1,19 +1,22 @@
> >  net = require('net.box')
> >  ---
> >  ...
> > +test_run = require('test_run').new()
> > +---
> > +...
> >  --
> >  -- gh-3856: wait_connected = false is ignored.
> >  --
> > -c = net.connect('8.8.8.8:123456', {wait_connected = false})
> > ----
> > -...
> > -c
> > +do                                                            \
> > +    c = net.connect('8.8.8.8:1234', {wait_connected = false}) \
> > +    return c                                                  \
> 
> You said you grab all connection fields, but you don't. This
> construction is 100% the same as
> 
>     c = net.connect('8.8.8.8:1234', {wait_connected = false})
> 
> Without 'do', 'return', and 'end'. In the original fix from
> Alexander Tu. I see that he returned c.state, not just c.
> But since you may need to call close() anyway, you probably
> should return
> 
>     c, c.state
> 
> So as to print the second value and close the first value.
> Another option - wrap all that into a function. Which calls
> close() inside, and returns the state.
> 

Ok, I've updated the test with the new function for it.

> > +end
> >  ---
> >  - opts:
> >      wait_connected: false
> >    host: 8.8.8.8
> >    state: initial
> > -  port: '123456'
> > +  port: '1234'
> >  ...
> >  c:close()

Re: [Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Alexander Turenko]

On Sat, Jun 20, 2020 at 07:50:39AM +0300, Alexander V. Tikhonov wrote:
> From: Alexander Turenko <alexander.turenko@tarantool.org>

Don't be hesitate to send it from youself, it would be more comfortable
for me. Even despite I participated in the investigation, the final
result including description and so is constructed by you.

Feel free to give credits to any participant within a commit message in
any form.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Alexander V. Tikhonov]

Hi Alexander, thanks for the comments, I've corrected all as you
suggested.

On Sun, Jun 21, 2020 at 09:41:20PM +0300, Alexander Turenko wrote:
> On Sat, Jun 20, 2020 at 07:50:39AM +0300, Alexander V. Tikhonov wrote:
> > From: Alexander Turenko <alexander.turenko@tarantool.org>
> 
> Don't be hesitate to send it from youself, it would be more comfortable
> for me. Even despite I participated in the investigation, the final
> result including description and so is constructed by you.
> 
> Feel free to give credits to any participant within a commit message in
> any form.
> 
> WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2] test: flaky box/net.box_wait_connected_gh-3856

[Kirill Yukhin]

Hello,

On 20 июн 07:50, Alexander V. Tikhonov wrote:
> From: Alexander Turenko <alexander.turenko@tarantool.org>
> 
> Found issue running test on FreeBSD VBox host:
> 
>  [011] --- box/net.box_wait_connected_gh-3856.result	Mon Jun 15 09:39:49 2020
>  [011] +++ box/net.box_wait_connected_gh-3856.reject	Fri May  8 08:23:30 2020
>  [011] @@ -12,7 +12,8 @@
>  [011]  - opts:
>  [011]      wait_connected: false
>  [011]    host: 8.8.8.8
>  [011] -  state: initial
>  [011] +  state: error
>  [011] +  error: Invalid argument
>  [011]    port: '123456'
>  [011]  ...
>  [011]  c:close()
> 
> The reason of the fail was that getaddrinfo() returned EIA_SERVICE for an
> incorrect TCP/IP port on FreeBSD, but crops it as modulo of 65536 on
> Linux/glibc. Checked with local script './getaddrinfo':
> 
>   (Linux/glibc) $ ./getaddrinfo 8.8.8.8 123456
>   ----
>   family: AF_INET
>   socktype: SOCK_STREAM
>   protocol: IPPROTO_TCP
>   host: 8.8.8.8
>   serv: 57920
> 
>   (FreeBSD) $ ./getaddrinfo 8.8.8.8 123456
>   getaddrinfo: Service was not recognized for socket type
> 
> So obvious fix is to change 123456 to something less or equal to
> 65535. Say, 1234.
> 
> The test depended on an order in which fibers were scheduled
> (net_box.connect() creates a separate fiber for connecting in background
> using fiber.create(), which yields). Unlikely our fiber were not get
> execution time during the connection attempt, so it was more like a
> formal thing.
> 
> But we can decrease probability of this situation even more if we'll
> grab all connection fields just when net_box.connect() returns, not
> after yield in console (which is due to waiting a next command from
> test-run).
> 
> Closes #5083
> 
> Reviewed-by: Alexander V. Tikhonov <avtikhon@tarantool.org>

I've checked your patch into 1.10, 2.3, 2.4 and master.

--
Regards, Kirill Yukhin