* [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI @ 2020-11-20 22:42 Vladislav Shpilevoy 2020-11-23 13:40 ` Igor Munkin 2020-12-18 17:28 ` Alexander V. Tikhonov 0 siblings, 2 replies; 5+ messages in thread From: Vladislav Shpilevoy @ 2020-11-20 22:42 UTC (permalink / raw) To: tarantool-patches, imun, avtikhon 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. Closes #4570 --- Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4570-swim-crash Issue: https://github.com/tarantool/tarantool/issues/4570 @ChangeLog * Fixed a crash in swim.quit() (gh-4570). src/lua/swim.c | 14 ++++++++++++++ src/lua/swim.lua | 5 +---- 2 files changed, 15 insertions(+), 4 deletions(-) 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); + swim_quit(s); + return 0; +} + void tarantool_lua_swim_init(struct lua_State *L) { @@ -98,6 +111,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 -- 2.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI 2020-11-20 22:42 [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI Vladislav Shpilevoy @ 2020-11-23 13:40 ` Igor Munkin 2020-11-23 22:10 ` Vladislav Shpilevoy 2020-12-18 17:28 ` Alexander V. Tikhonov 1 sibling, 1 reply; 5+ messages in thread From: Igor Munkin @ 2020-11-23 13:40 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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". > > Closes #4570 > --- > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4570-swim-crash > Issue: https://github.com/tarantool/tarantool/issues/4570 > > @ChangeLog > * Fixed a crash in swim.quit() (gh-4570). > > src/lua/swim.c | 14 ++++++++++++++ > src/lua/swim.lua | 5 +---- > 2 files changed, 15 insertions(+), 4 deletions(-) > > 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. > + swim_quit(s); > + return 0; > +} > + > void > tarantool_lua_swim_init(struct lua_State *L) > { <snipped> > -- > 2.24.3 (Apple Git-128) > -- Best regards, IM ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI 2020-11-23 13:40 ` Igor Munkin @ 2020-11-23 22:10 ` Vladislav Shpilevoy 0 siblings, 0 replies; 5+ messages in thread From: Vladislav Shpilevoy @ 2020-11-23 22:10 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI 2020-11-20 22:42 [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI Vladislav Shpilevoy 2020-11-23 13:40 ` Igor Munkin @ 2020-12-18 17:28 ` Alexander V. Tikhonov 2020-12-20 17:21 ` Vladislav Shpilevoy 1 sibling, 1 reply; 5+ messages in thread From: Alexander V. Tikhonov @ 2020-12-18 17:28 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi Vlad, thanks for the patch, as I see no new degradation found in gitlab-ci testing commit criteria pipeline [1], patch LGTM. [1] - https://gitlab.com/tarantool/tarantool/-/pipelines/220237031 On Fri, Nov 20, 2020 at 11:42:07PM +0100, 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. > > Closes #4570 > --- > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4570-swim-crash > Issue: https://github.com/tarantool/tarantool/issues/4570 > > @ChangeLog > * Fixed a crash in swim.quit() (gh-4570). > > src/lua/swim.c | 14 ++++++++++++++ > src/lua/swim.lua | 5 +---- > 2 files changed, 15 insertions(+), 4 deletions(-) > > 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); > + swim_quit(s); > + return 0; > +} > + > void > tarantool_lua_swim_init(struct lua_State *L) > { > @@ -98,6 +111,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 > -- > 2.24.3 (Apple Git-128) > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI 2020-12-18 17:28 ` Alexander V. Tikhonov @ 2020-12-20 17:21 ` Vladislav Shpilevoy 0 siblings, 0 replies; 5+ messages in thread From: Vladislav Shpilevoy @ 2020-12-20 17:21 UTC (permalink / raw) To: Alexander V. Tikhonov; +Cc: tarantool-patches Pushed to master, 2.6, and 2.5. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-20 17:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 22:42 [Tarantool-patches] [PATCH 1/1] swim: don't call swim_quit via FFI Vladislav Shpilevoy 2020-11-23 13:40 ` Igor Munkin 2020-11-23 22:10 ` Vladislav Shpilevoy 2020-12-18 17:28 ` Alexander V. Tikhonov 2020-12-20 17:21 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox