[tarantool-patches] Re: [PATCH V4] Prohibit router/storage configuration from different fibers

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes! See 3 comments below.

> diff --git a/test/router/router.result b/test/router/router.result
> index 000307d..5ce48b1 100644
> --- a/test/router/router.result
> +++ b/test/router/router.result
> @@ -1130,6 +1130,44 @@ vshard.router.static:route(1):callro('echo',
> {'some_data'})
>   - null
>   - null
>   ...

1. Please, do not hurry. Looks like you did not run a test.

This is what I got for router/router.test.lua:

         f = function()
         ---
         - error: '[string "f = function() "]:1: ''end'' expected near ''<eof>'''
         ...
             fiber.create(function()
         ---
         - error: '[string "    fiber.create(function() "]:1: ''end'' expected near ''<eof>'''
         ...
                 local status, err = pcall(vshard.router.cfg, cfg)
         ---
         ...
                 c:put(status and true or false)
         ---
         - error: '[string "return         c:put(status and true or false) "]:1: variable ''status''
             is not declared'
         ...
             end)
         ---
         - error: '[string "    end) "]:1: ''<eof>'' expected near ''end'''
         ...
         end
         ---
         - error: '[string "end "]:1: ''<eof>'' expected near ''end'''
         ...
         f()
         ---
         - error: '[string "return f() "]:1: variable ''f'' is not declared'
         ...
         f()
         ---
         - error: '[string "return f() "]:1: variable ''f'' is not declared'
         ...
         c:get()

Obviously, your test is not about Lua syntax.

The error is trivial so I just inlined multiline functions, but got more errors
like this:

	[001]  vshard.storage.cfg(cfg, util.name_to_uuid.storage_1_a)
	[001]  ---
	[001] +- true
	[001]  ...

Looks like you started to return a boolean from storage.cfg.
Please, don't. It changes behavior of a public API with no
reason.

This error is trivial as well, but the next is extra strange
and the same as in the previous review: I have removed this line

	vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = 0

And got no errors. So the test passes regardless of ERRINJ_MULTIPLE_CFG
value. Why? It shall not be so. Such a test makes no sense if it
tests nothing.

I understand your wish to push the patch as soon as possible, but
please, be more attentive. Elaborate each line of your code before
sending it to the mailing list. Run the entire test suite 3-4 times.

> +-- gh-140: prohibit vshard.router/storage.cfg from multiple fibers.
> +cfg = {sharding = require('localcfg').sharding}
> +---
> +...
> +vshard.router.cfg(cfg)
> +---
> +...
> +c = fiber.channel(2)
> +---
> +...
> +vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = 0
> +---
> +...
> +f = function()
> +    fiber.create(function()
> +        local status, err = pcall(vshard.router.cfg, cfg)
> +        c:put(status and true or false)

2. 'status' is boolean by definition of pcall() function. So
here you do <x and true or false>. Then look into this:
https://en.wikipedia.org/wiki/Boolean_algebra and use monotone
laws:

     x and true = x
     x or false = x

Apply it to your expression:

     status and true or false = (status and true) or false =
     = status or false = status

So just do:

     c:put(status)

What is more, you can do this:

c:put((pcall(vshard.router.cfg, cfg))) - here I
wrapped pcall() into additional () to cut a second
retval off - this is nice Lua feature.

All the same comments for storage.test.lua.

> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 7573801..93bef77 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -28,6 +28,7 @@ if not M then
>           ---------------- Common module attributes ----------------
>           errinj = {
>               ERRINJ_CFG = false,
> +            ERRINJ_MULTIPLE_CFG = nil,
>               ERRINJ_FAILOVER_CHANGE_CFG = false,
>               ERRINJ_RELOAD = false,
>               ERRINJ_LONG_DISCOVERY = false,
> @@ -53,6 +54,8 @@ local ROUTER_TEMPLATE = {
>           name = nil,
>           -- The last passed configuration.
>           current_cfg = nil,
> +        -- Fiber channel as the locker to prevent multiple config.
> +        cfg_lock = nil,
>           -- Time to outdate old objects on reload.
>           connection_outdate_delay = nil,
>           -- Bucket map cache.
> @@ -594,6 +597,34 @@ local function router_cfg(router, cfg, is_reload)
>       end
>   end
> 
> +--
> +-- Wrapper around 'router_cfg' function to provide
> +-- locking beafore / unlocking after pcall(router_cfg, router, ...),
> +-- so it prevents 'router_cfg' been called from diffenrent fibers.

3. Typos: 'beafore', 'diffenrent'. The same for storage cfg wrapper.

> +-- @param router Router instance to be configured.
> +-- @param ... Arguments which would be passed to the `router_cfg`.
> +-- @retval true, ... on success
> +--
> +local function cfg_wrapper(router, ...)
> +    local status, err
> +    if M.errinj.ERRINJ_MULTIPLE_CFG then
> +        M.errinj.ERRINJ_MULTIPLE_CFG = M.errinj.ERRINJ_MULTIPLE_CFG + 1
> +        if M.errinj.ERRINJ_MULTIPLE_CFG > 1 then
> +            error('Multiple cfgs at the same time')

This error should appear in a test.

> +        end
> +        status, err = pcall(router_cfg, router, ...)
> +        M.errinj.ERRINJ_MULTIPLE_CFG = M.errinj.ERRINJ_MULTIPLE_CFG - 1
> +    else
> +        router.cfg_lock:put(true)
> +        status, err = pcall(router_cfg, router, ...)
> +        router.cfg_lock:get()
> +    end
> +    if not status then
> +        error(err)
> +    end
> +    return true
> +end