From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Igor Munkin <imun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI Date: Mon, 23 Nov 2020 23:10:16 +0100 [thread overview] Message-ID: <a8b03f2c-7065-77f4-961b-74ac6da217fc@tarantool.org> (raw) In-Reply-To: <20201123134030.GB14086@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 <ctypeid> equals to <ctid_swim_ptr> 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
next prev parent reply other threads:[~2020-11-23 22:10 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-20 22:42 Vladislav Shpilevoy 2020-11-23 13:40 ` Igor Munkin 2020-11-23 22:10 ` Vladislav Shpilevoy [this message] 2020-12-18 17:28 ` Alexander V. Tikhonov 2020-12-20 17:21 ` Vladislav Shpilevoy
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: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a8b03f2c-7065-77f4-961b-74ac6da217fc@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=imun@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 1/1] swim: don'\''t call swim_quit via FFI' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * 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