Re: [Tarantool-patches] [PATCH] serializer: serialize recursive structures

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hello!

thanks for the patch!

1. I have reverted all changes and test passed.

2. In commit message you declares recursive structures support.
What is the max supported depth of recursive structure?
I believe it is worth to describe possible limitations in commit message.

See inline

On 11.03.2021 08:49, Roman Khabibov via Tarantool-patches wrote:
> Fix bug with bus error during serializing of recursive
> structures.
>
> Closes #3228
> ---
>
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3228-visited-set
> Issue: https://github.com/tarantool/tarantool/issues/3228
>
>   src/box/lua/call.c                            |   6 +-
>   src/box/lua/execute.c                         |   2 +-
>   src/box/lua/serialize_lua.c                   |  96 ++------
>   src/box/lua/tuple.c                           |   2 +-
>   src/box/sql/func.c                            |   2 +-
>   src/lua/msgpack.c                             |  10 +-
>   src/lua/pickle.c                              |   2 +-
>   src/lua/utils.c                               | 226 ++++++++++++++++--
>   src/lua/utils.h                               |  43 +++-
>   ...-3228-serializer-look-for-recursion.result |  67 ++++++
>   ...228-serializer-look-for-recursion.test.lua |  17 ++
>   test/swim/swim.result                         |  18 +-
>   third_party/lua-cjson/lua_cjson.c             |   4 +-
>   third_party/lua-yaml/lyaml.cc                 |  88 +++----
>   14 files changed, 390 insertions(+), 193 deletions(-)
>   create mode 100644 test/app/gh-3228-serializer-look-for-recursion.result
>   create mode 100644 test/app/gh-3228-serializer-look-for-recursion.test.lua

<snipped>

> diff --git a/test/app/gh-3228-serializer-look-for-recursion.test.lua b/test/app/gh-3228-serializer-look-for-recursion.test.lua
> new file mode 100644
> index 000000000..1c9b0375f
> --- /dev/null
> +++ b/test/app/gh-3228-serializer-look-for-recursion.test.lua
> @@ -0,0 +1,17 @@
> +test_run = require('test_run').new()
> +
> +--
> +-- gh-3228: Check the error message in the case of a __serialize
> +-- function generating infinite recursion.
> +--
> +setmetatable({}, {__serialize = function(a) return a end})
> +setmetatable({}, {__serialize = function(a) return {a} end})
> +setmetatable({}, {__serialize = function(a) return {a, a} end})
> +setmetatable({}, {__serialize = function(a) return {a, a, a} end})
> +setmetatable({}, {__serialize = function(a) return {{a, a}, a} end})
> +setmetatable({}, {__serialize = function(a) return {a, 1} end})
> +setmetatable({}, {__serialize = function(a) return {{{{a}}}} end})
> +setmetatable({}, {__serialize = function(a) return {{{{1}}}} end})

3. Are you sure 4 levels depth is enough to check recursive structure?

What is about testing max depth?


> +b = {}
> +setmetatable({b}, {__serialize = function(a) return {a_1 = a, a_2 = a, b_1 = b, b_2 = b} end})
> +setmetatable({b}, {__serialize = function(a) return {a_1 = a, a_2 = {a, b}, b = b} end})
> diff --git a/test/swim/swim.result b/test/swim/swim.result
> index bfc83c143..539131677 100644
> --- a/test/swim/swim.result
> +++ b/test/swim/swim.result
> @@ -1322,16 +1322,13 @@ m_list
>       incarnation: cdata {generation = 0ULL, version = 1ULL}
>       uuid: 00000000-0000-1000-8000-000000000002
>       payload_size: 0
> -  - uri: 127.0.0.1:<port>
> -    status: alive
> -    incarnation: cdata {generation = 0ULL, version = 2ULL}
> -    uuid: 00000000-0000-1000-8000-000000000001
> -    payload_size: 8
> -  - uri: 127.0.0.1:<port>
> +  - &0
> +    uri: 127.0.0.1:<port>
>       status: alive
>       incarnation: cdata {generation = 0ULL, version = 2ULL}
>       uuid: 00000000-0000-1000-8000-000000000001
>       payload_size: 8
> +  - *0
>   ...
>   e_list
>   ---
> @@ -1374,16 +1371,13 @@ fiber.sleep(0)
>   -- Two events - status update to 'left', and 'drop'.
>   m_list
>   ---
> -- - uri: 127.0.0.1:<port>
> -    status: left
> -    incarnation: cdata {generation = 0ULL, version = 1ULL}
> -    uuid: 00000000-0000-1000-8000-000000000002
> -    payload_size: 0
> -  - uri: 127.0.0.1:<port>
> +- - &0
> +    uri: 127.0.0.1:<port>
>       status: left
>       incarnation: cdata {generation = 0ULL, version = 1ULL}
>       uuid: 00000000-0000-1000-8000-000000000002
>       payload_size: 0
> +  - *0
>   ...
>   e_list
>   ---

4. How changes in swim test related to patch?

It is not clear from commit message.

<snipped>