Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: Cyrill Gorcunov <gorcunov@tarantool.org>,
Subject: Re: [Tarantool-patches] [PATCH 0/4] RFC: Isolate serializer helpers
Date: Mon, 5 Jul 2021 09:30:47 +0300	[thread overview]
Message-ID: <20210705063047.uwg6aby2dg44wqbm@tkn_work_nb> (raw)
In-Reply-To: <d2bfe28b-49dc-4bf2-a64a-58042a197c0e@tarantool.org>

On Sun, Jul 04, 2021 at 03:09:07PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> On 23.06.2021 21:12, Alexander Turenko via Tarantool-patches wrote:
> > Moved the serializer helpers into its own compilation unit, add some
> > comments and a basic test: everything is just to simplify diving into
> > this code.
> > 
> > Guys, please, look, whether it seems useful enough to include into
> > tarantool's mainline? Should we name it serializer.[ch] or
> > somehow like serializer_helpers.[ch]?
> > 
> > Part of https://github.com/tarantool/tarantool/issues/3228
> Are you sure you need to fix it? It looks like a regular leg shooting.
> It might be simple to detect in the case described by Mons, but what if
> the recursion is not so easily visible?
> 	setmetatable({},{
> 		__serialize = function(a)
> 			return {{{{a}}}}
> 		end
> 	})
> You would need to use recursion detection algorithms like the one
> we used to ask on interviews. And I am not sure it is worth it if
> it can't be done in a simple way.

I think it worth to rearrange the code and add a test disregarding
whether we'll decide to fix or leave the problem.

I'll update the issue on the week with description of all problems found
around __serialize (see at end of the email as well). After this I'll
ask Roman to update its patch (I'll add a checklist what should be done
and how). I'll keep you in CC for those discussions. So you'll have
ability to say 'it looks to complex' at any stage.

In my opinion, it is highly undesirable to get segfault (or even a Lua
error) from a serializer, because it is often used for logging. More or
less correct result is better than fail. Even if the passed Lua object
is ill-formed in some way. (However, sure, I want to keep the code as
readable as possible and I would not accept a solution that is hard for
me to dive into. I hope we'll implement something well balanced.)

To be honest, even our usual "unsupported Lua type 'function'" error
(which is raised for a function if `encode_use_tostring` is not `true`)
is often undesirable. Raw idea: provide a helper like
`yaml.encode_noxc()`, which will never raise an error and will be
suitable for logging in the general case (it'll set
`encode_use_tostring` under the hood).

WBR, Alexander Turenko.


Sure, there are two problems with __serialize, which lead to segfault:

- recursion within single Lua object serialization;
- recursion over several Lua objects.

But there is one problem of another kind.

A return value of __serialize does not participate in references search.

 | local x = {whoami = 'x'}
 | yaml.encode({
 |     foo = x,
 |     bar = setmetatable({}, {__serialize = function(_) return x end})
 | })

** now **
 | ---
 | foo:
 |   whoami: x
 | bar:
 |   whoami: x
 | ...

** should be **
 | ---
 | foo: &1
 |   whoami: x
 | bar: *1
 | ...

  reply	other threads:[~2021-07-05  6:31 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23 19:12 Alexander Turenko via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 1/4] lua: move serializer helpers into its own file Alexander Turenko via Tarantool-patches
2021-07-04 13:10   ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  6:30     ` Alexander Turenko via Tarantool-patches
2021-07-05 20:59       ` Vladislav Shpilevoy via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 2/4] lua: move luaL_newserializer() comment into header Alexander Turenko via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 3/4] lua: split serializer functions into sections Alexander Turenko via Tarantool-patches
2021-06-23 19:12 ` [Tarantool-patches] [PATCH 4/4] test: add a basic unit test for serializer helpers Alexander Turenko via Tarantool-patches
2021-06-24  6:17 ` [Tarantool-patches] [PATCH 0/4] RFC: Isolate " Cyrill Gorcunov via Tarantool-patches
2021-06-28  6:31 ` Cyrill Gorcunov via Tarantool-patches
2021-07-04 13:09 ` Vladislav Shpilevoy via Tarantool-patches
2021-07-05  6:30   ` Alexander Turenko via Tarantool-patches [this message]
2021-07-07 10:08 ` Alexander Turenko via Tarantool-patches
2021-07-07 19:09   ` Alexander Turenko via Tarantool-patches
2021-07-07 22:16     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-12  7:51       ` Alexander Turenko via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210705063047.uwg6aby2dg44wqbm@tkn_work_nb \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=gorcunov@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 0/4] RFC: Isolate serializer helpers' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox