

<HTML><BODY><div>Hi team, </div><div> </div><div>QA LGTM!</div><div> </div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Vitaliia Ioffe</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Пятница, 13 августа 2021, 10:32 +03:00 от Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16288399341647753424_BODY">Igor,<br><br>On 12.08.21, Igor Munkin wrote:<br>> Sergey,<br>><br>> Thanks for the fixes! LGTM, with a several typos in the commit message<br>> mentioned below. Moreover, please rebase your branch on the current<br>> master to check nothing is broken.<br><br>Branch is rebased, fixes are done, and branch is force-pushed :).<br><br>><br>> On 02.08.21, Sergey Kaplun wrote:<br>> > Hi, Igor!<br>> ><br>> > Thanks for the review!<br>> ><br>><br>> <snipped><br>><br>> ><br>> > The new commit message is the following:<br>> ><br>> > ===================================================================<br>> > lua: refactor port_lua_do_dump and encode_lua_call<br>> ><br>> > The old code flow was the following:<br>> ><br>> > 1) `struct port_lua` given to `port_lua_do_dump()` has Lua stack with<br>> > arguments to encode to MessagePack.<br>> ><br>> > 2) The main coroutine `tarantool_L` is used to call `encode_lua_call()`<br>> > or `encode_lua_call_16`() via `lua_cpcall()`.<br>> ><br>> > 3) Objects on port coroutine are encoded via `luamp_encode()` or<br>> > `luamp_encode_call16()`.<br>> ><br>> > 4) This encoding may raise an error on unprotected `port->L` coroutine.<br>> > This coroutine has no protected frame on it and this call should fail<br>> > in pure Lua.<br>> ><br>> > Calling anything on unprotected coroutine is not allowed in Lua [1]:<br>> ><br>> > | If an error happens outside any protected environment, Lua calls a<br>> > | panic function<br>> ><br>> > Lua 5.1 sets protection only for specific lua_State [2] and calls a<br>> > panic function if we raise an error on unprotected lua_State [3].<br>> ><br>> > Nevertheless, no panic occurs now due to two facts:<br>> > * The first one is LuaJIT's support of C++ exception handling [4] that<br>> > allows to raise an error in Lua and catch it in C++ or vice versa. But<br>> > documentation still doesn't allow raising errors on unprotected<br>> > coroutines (at least we must use try-catch block).<br>> > * The second one is the patch made in LuaJIT to restore currently<br>> > executed coroutine, when C function or fast function raises an<br>> > error [5][6] (see the related issue here [7][8]).<br>> ><br>> > For these reasons, when an error occurs, the unwinder searches and finds<br>> > the C-protected stack frame from the `lua_cpcall()` for `tarantool_L`<br>> > coroutine and unwinds until that point (without aforementioned patches<br>> > LuaJIT just calls a panic function and exit).<br>><br>> Typo: s/exit/exits/.<br><br>Fixed.<br><br>><br>> ><br>> > If an error is raised, and `lua_cpcall()` returns not `LUA_OK`, then the<br>> > error from `port->L` coroutine is converted into a Tarantool error and a<br>> > diagnostic is set.<br>> ><br>> > The auxiliary usage of `tarantool_L` coroutine doesn't respect Lua<br>> > idiomatic of usage. Internal unwinder used on M1 is not such flexible,<br>><br>> Typo: Too much "usage", so I propose the following wording for the<br>> previous sentence:<br>> | Such auxiliary usage of `tarantool_L` is not idiomatic for Lua.<br><br>Done.<br><br>><br>> > so such misuse leads to panic call. Also the `tarantool_L` usage is<br>> > redundant. So this patch drops it and uses only minor coroutine instead<br>><br>> Typo: Again, not minor coroutine, but port coroutine (as we agreed in<br>> the previous review).<br><br>Fixed.<br><br>><br>> > with `lua_pcall()`.<br>> ><br>> > Functions to encode are saved as entrance in the `LUA_REGISTRY` table to<br>><br>> Typo: s/saved as entrance in/saved to/.<br><br>Fixed.<br><br>><br>> > reduce GC pressure, like it is done for other handlers [9].<br>> ><br>> > [1]: <a href="https://www.lua.org/manual/5.2/manual.html#4.6" target="_blank">https://www.lua.org/manual/5.2/manual.html#4.6</a><br>> > [2]: <a href="https://www.lua.org/source/5.1/lstate.h.html#lua_State" target="_blank">https://www.lua.org/source/5.1/lstate.h.html#lua_State</a><br>> > [3]: <a href="https://www.lua.org/source/5.1/ldo.c.html#luaD_throw" target="_blank">https://www.lua.org/source/5.1/ldo.c.html#luaD_throw</a><br>> > [4]: <a href="https://luajit.org/extensions.html#exceptions" target="_blank">https://luajit.org/extensions.html#exceptions</a><br>> > [5]: <a href="https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0" target="_blank">https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0</a><br>> > [6]: <a href="https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f" target="_blank">https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f</a><br>> > [7]: <a href="https://github.com/tarantool/tarantool/issues/1516" target="_blank">https://github.com/tarantool/tarantool/issues/1516</a><br>> > [8]: <a href="https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21" target="_blank">https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21</a><br>> > [9]: <a href="https://github.com/tarantool/tarantool/commit/e88c0d21ab765d4c53bed2437c49d77b3ffe4216" target="_blank">https://github.com/tarantool/tarantool/commit/e88c0d21ab765d4c53bed2437c49d77b3ffe4216</a><br>> ><br>> > Closes #6248<br>> > Closes #4617<br>> > ===================================================================<br>> ><br>> > > > ---<br>> > > ><br>> > > > Branch: <a href="https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-refactor-lua-call" target="_blank">https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-refactor-lua-call</a><br><br><snipped><br><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>