Fix hotreloadability

[rosik]

Gracefully handle hotreload. It didn't work because postload.lua always tried to call ffi.metatype(), but luajit doesn't allow to do it twice for the same type.

Fixes #9.

[Totktonada]

Thanks! I got the idea and it looks okay on the first glance. I'll look deeper soon.

[olegrok]

Thanks for your patch. I left several comments. They are quite minor and some of them maybe could be skipped.

[olegrok]

I'm not sure that "tuple" defined in global affect something. It seems an artefact of luaL_register usage.

[Totktonada]

Hm. I think it should not be so, it is the mistake.

Anyway, while the global is set at the module initialization, it looks meaningful to drop it in the test.

[Totktonada]

I'll propose a fix after PR #15. Too much PRs in the fly :)

[Totktonada]

The draft commit is a56ff5d (as said, I'll open a PR later).

[Totktonada]

Opened PR #16.

[olegrok]

In case when this line fails all following tests will be stopped. I suggest to use something like:

local ok, tuple_keydef = pcall(require, 'tuple.keydef')
test:ok(ok)

[Totktonada]

However a next test case definitely will fail due to tuple_keydef = nil. We can move the test to a separate file or pass it over.

[Totktonada]

In PR #15 I moved those test cases into its own file. I wrapped require() into pcall(), where the goal of a test case is to verify, whether it succeeds. However I found it unconvenient to wrap a preparation code into pcall() (or wrap each test case into pcall()). So, in fact, nothing changes: if one test case will fail, others will not run.

If we'll move toward luatest, it'll handle this problem for us. But prior that I would disregard it.

[olegrok]

Not sure it's related to current patch and current problem. But is it possible to eliminate this call at all (https://github.com/tarantool/tarantool/blob/35dd826460e876c6cdb1ab7395ef7a5064615d2b/src/lua/utils.c#L141)?

E.g. initialize CTID_STRUCT_TUPLE_KEY_DEF_REF by default (with e.g. 0) and then call luaL_ctypeid only if value is 0.

[Totktonada]

To be honest, I'm not sure I got the idea. Do you propose to set it at the first load and don't touch on next reloads?

[olegrok]

Do you propose to set it at the first load and don't touch on next reloads?

Yes

[Totktonada]

Hm. I don't know, whether I should do or don't. The behaviour is exactly same. I left it as is in PR #15, it is out of scope there.

[Totktonada]

I proposed an alternative fix in PR #15.

My doubts about this implementation (PR #10) are the following:

1. It produces implicit dependencies between the Lua/C code and the Lua code: we should call all those ffi.cdef() / ffi.metatype() / luaL_ctypeid() in a particular order. (That's why luaL_ctypeid() is moved here. And it makes the postload code being a kind of… middle load?)
2. It leans on persence of the struct tuple_keydef ctype as on indicator that we set a metatype on it. So, it is impossible to declare the ctype prior to a first load of the module. (BTW, it is easy to fix in this implementation too.)
3. It moves more code to the postload.lua file, but I would prefer to keep it as small as possible. A module is more readable, when tightly coupled parts are not spread across different files. Here I need the Lua part only to call ffi.metatype() (there is no corresponding C API). But the 'direct' bundling into the Lua/C code using loadstring() would look even worse. I think that currrent solution with the separate postload.lua (which is bundled into .so) is the least evil way, while we'll keep the postload.lua small and simple.

Since all those points may be eliminated, I think that they should be eliminated. I did it in PR #15.

[Totktonada]

Superseded by PR #15.