From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 98733469710 for ; Tue, 24 Nov 2020 01:10:18 +0300 (MSK) References: <8f87d28e7b1dc259fa0b1f230c0c6a5b25573bf1.1605912044.git.v.shpilevoy@tarantool.org> <20201123134030.GB14086@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 23 Nov 2020 23:10:16 +0100 MIME-Version: 1.0 In-Reply-To: <20201123134030.GB14086@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review! On 23.11.2020 14:40, Igor Munkin wrote: > Vlad, > > Thanks for the patch! LGTM, with a couple nits below. > > On 20.11.20, Vladislav Shpilevoy wrote: >> swim_quit yields, because it joins the event handler fiber. Hence >> it can't be called via FFI, where a yield leads to undefined >> behaviour. > > Minor: I wouldn't call this UB, let's reword it "where a yield might > lead to the platform panic". Agree, applied. >> diff --git a/src/lua/swim.c b/src/lua/swim.c >> index ae916bf78..b9c9dd635 100644 >> --- a/src/lua/swim.c >> +++ b/src/lua/swim.c >> @@ -88,6 +88,19 @@ lua_swim_delete(struct lua_State *L) >> return 0; >> } >> >> +/** >> + * Gracefully leave the cluster, broadcast a notification, and delete the SWIM >> + * instance. It is not FFI, because this operation yields. >> + */ >> +static int >> +lua_swim_quit(struct lua_State *L) >> +{ >> + uint32_t ctypeid; >> + struct swim *s = *(struct swim **) luaL_checkcdata(L, 1, &ctypeid); > > Minor: I don't know whether it's common practice in Tarantool, but > assert that equals to would be nice here. > Feel free to ignore. I added it to quit and delete. The new patch is below: ==================== swim: don't call swim_quit via FFI swim_quit yields, because it joins the event handler fiber. Hence it can't be called via FFI, where a yield might lead to platform panic. Closes #4570 diff --git a/src/lua/swim.c b/src/lua/swim.c index ae916bf78..cfce50e1a 100644 --- a/src/lua/swim.c +++ b/src/lua/swim.c @@ -84,10 +84,25 @@ lua_swim_delete(struct lua_State *L) { uint32_t ctypeid; struct swim *s = *(struct swim **) luaL_checkcdata(L, 1, &ctypeid); + assert(ctypeid == ctid_swim_ptr); swim_delete(s); return 0; } +/** + * Gracefully leave the cluster, broadcast a notification, and delete the SWIM + * instance. It is not FFI, because this operation yields. + */ +static int +lua_swim_quit(struct lua_State *L) +{ + uint32_t ctypeid; + struct swim *s = *(struct swim **) luaL_checkcdata(L, 1, &ctypeid); + assert(ctypeid == ctid_swim_ptr); + swim_quit(s); + return 0; +} + void tarantool_lua_swim_init(struct lua_State *L) { @@ -98,6 +113,7 @@ tarantool_lua_swim_init(struct lua_State *L) static const struct luaL_Reg lua_swim_internal_methods [] = { {"swim_new", lua_swim_new}, {"swim_delete", lua_swim_delete}, + {"swim_quit", lua_swim_quit}, {"swim_on_member_event", lua_swim_on_member_event}, {NULL, NULL} }; diff --git a/src/lua/swim.lua b/src/lua/swim.lua index c1ab1c5c3..1da55337a 100644 --- a/src/lua/swim.lua +++ b/src/lua/swim.lua @@ -74,9 +74,6 @@ ffi.cdef[[ int swim_size(const struct swim *swim); - void - swim_quit(struct swim *swim); - struct swim_member * swim_self(struct swim *swim); @@ -519,7 +516,7 @@ end -- local function swim_quit(s) local ptr = swim_check_instance(s, 'swim:quit') - capi.swim_quit(ffi.gc(ptr, nil)) + internal.swim_quit(ffi.gc(ptr, nil)) s.ptr = nil setmetatable(s, swim_mt_deleted) end