From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 7056A6EC40; Fri, 13 Aug 2021 10:31:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7056A6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628839919; bh=tGHgG84gaqBWNNG3E/mTmXuUgWhCkwnde7pMvFdR25k=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=AbUgobfO25RoIenmqOG6Ei3a/5r0VMxl3rCyisnJUORkntccjFonrcMc9TICbsVXm p9xsi/Sq3HfnART1xP0ni4VYdGhIbUkKUNGBffGzpVjsDF+4S9xO8IHiYqeFHnzAWE opFJMIHpvHs9CKD5m1hi2vuMWTrY1u0RdaUq07IU= Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A67136EC40 for ; Fri, 13 Aug 2021 10:31:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A67136EC40 Received: by smtp57.i.mail.ru with esmtpa (envelope-from ) id 1mERfY-000296-Fj; Fri, 13 Aug 2021 10:31:56 +0300 Date: Fri, 13 Aug 2021 10:30:40 +0300 To: Igor Munkin Cc: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Message-ID: References: <20210618181416.25454-1-skaplun@tarantool.org> <20210801123417.GA27855@tarantool.org> <20210812173516.GN27855@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210812173516.GN27855@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD910164DC12A5633065676A9727AC27C74182A05F53808504099F1810C6CAEC39C76D345DD1723CFA6F77C0E1C1751A5178321C7F07176352B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B8498AC83B273C12EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063771C846A5973DEE7E8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B01316C19D95DAB173E7100A11990828117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A55896078E03636271FFB30F8124BB08108310C7557FD2FC81D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA750E14360347543F58410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34060C3C6DE316ECE429D63E6D64AD0B88F10362547BF237D0DE466A6E1153BD722C9ED0E838796D061D7E09C32AA3244C64316CDB83F1FCA1D20E2755B608165F1E098CBE561D6343FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj0dLV0c3jbkwdw+chfUKbXw== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB403858274F9EA5FB4466788594F3DA88E6CDBA43939999A30F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Igor, On 12.08.21, Igor Munkin wrote: > Sergey, > > Thanks for the fixes! LGTM, with a several typos in the commit message > mentioned below. Moreover, please rebase your branch on the current > master to check nothing is broken. Branch is rebased, fixes are done, and branch is force-pushed :). > > On 02.08.21, Sergey Kaplun wrote: > > Hi, Igor! > > > > Thanks for the review! > > > > > > > > > The new commit message is the following: > > > > =================================================================== > > lua: refactor port_lua_do_dump and encode_lua_call > > > > The old code flow was the following: > > > > 1) `struct port_lua` given to `port_lua_do_dump()` has Lua stack with > > arguments to encode to MessagePack. > > > > 2) The main coroutine `tarantool_L` is used to call `encode_lua_call()` > > or `encode_lua_call_16`() via `lua_cpcall()`. > > > > 3) Objects on port coroutine are encoded via `luamp_encode()` or > > `luamp_encode_call16()`. > > > > 4) This encoding may raise an error on unprotected `port->L` coroutine. > > This coroutine has no protected frame on it and this call should fail > > in pure Lua. > > > > Calling anything on unprotected coroutine is not allowed in Lua [1]: > > > > | If an error happens outside any protected environment, Lua calls a > > | panic function > > > > Lua 5.1 sets protection only for specific lua_State [2] and calls a > > panic function if we raise an error on unprotected lua_State [3]. > > > > Nevertheless, no panic occurs now due to two facts: > > * The first one is LuaJIT's support of C++ exception handling [4] that > > allows to raise an error in Lua and catch it in C++ or vice versa. But > > documentation still doesn't allow raising errors on unprotected > > coroutines (at least we must use try-catch block). > > * The second one is the patch made in LuaJIT to restore currently > > executed coroutine, when C function or fast function raises an > > error [5][6] (see the related issue here [7][8]). > > > > For these reasons, when an error occurs, the unwinder searches and finds > > the C-protected stack frame from the `lua_cpcall()` for `tarantool_L` > > coroutine and unwinds until that point (without aforementioned patches > > LuaJIT just calls a panic function and exit). > > Typo: s/exit/exits/. Fixed. > > > > > If an error is raised, and `lua_cpcall()` returns not `LUA_OK`, then the > > error from `port->L` coroutine is converted into a Tarantool error and a > > diagnostic is set. > > > > The auxiliary usage of `tarantool_L` coroutine doesn't respect Lua > > idiomatic of usage. Internal unwinder used on M1 is not such flexible, > > Typo: Too much "usage", so I propose the following wording for the > previous sentence: > | Such auxiliary usage of `tarantool_L` is not idiomatic for Lua. Done. > > > so such misuse leads to panic call. Also the `tarantool_L` usage is > > redundant. So this patch drops it and uses only minor coroutine instead > > Typo: Again, not minor coroutine, but port coroutine (as we agreed in > the previous review). Fixed. > > > with `lua_pcall()`. > > > > Functions to encode are saved as entrance in the `LUA_REGISTRY` table to > > Typo: s/saved as entrance in/saved to/. Fixed. > > > reduce GC pressure, like it is done for other handlers [9]. > > > > [1]: https://www.lua.org/manual/5.2/manual.html#4.6 > > [2]: https://www.lua.org/source/5.1/lstate.h.html#lua_State > > [3]: https://www.lua.org/source/5.1/ldo.c.html#luaD_throw > > [4]: https://luajit.org/extensions.html#exceptions > > [5]: https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 > > [6]: https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f > > [7]: https://github.com/tarantool/tarantool/issues/1516 > > [8]: https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21 > > [9]: https://github.com/tarantool/tarantool/commit/e88c0d21ab765d4c53bed2437c49d77b3ffe4216 > > > > Closes #6248 > > Closes #4617 > > =================================================================== > > > > > > --- > > > > > > > > Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-refactor-lua-call -- Best regards, Sergey Kaplun