[tarantool-patches] [PATCH 0/4] SWIM amendments

[tarantool-patches] [PATCH 0/4] SWIM amendments

[Vladislav Shpilevoy]

A set of SWIM follow ups related to a couple of tickets. Most of them are
bugfixes.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/swim-follow-ups

Vladislav Shpilevoy (4):
  swim: fix a leak when a trigger is installed
  swim: fix a dangerous yield in ffi.gc
  swim: fix inability to set generation only
  swim: default generation is timestamp

 src/lua/swim.lua        | 40 +++++++++++++++++---
 test/swim/swim.result   | 81 +++++++++++++++++++++++++++++++++++------
 test/swim/swim.test.lua | 50 +++++++++++++++++++------
 3 files changed, 142 insertions(+), 29 deletions(-)

-- 
2.20.1 (Apple Git-117)

[tarantool-patches] [PATCH 1/4] swim: fix a leak when a trigger is installed

[Vladislav Shpilevoy]

SWIM wraps user triggers to prepare arguments. The wrapper
function kept a reference to SWIM object, and prevented its
automatic deletion at GC.

The patch makes this reference weak.

Follow up #4250
---
 src/lua/swim.lua        | 13 ++++++++++---
 test/swim/swim.result   | 21 +++++++++++++++++++++
 test/swim/swim.test.lua | 10 ++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index b6d826ca0..bae6c0da6 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -786,10 +786,17 @@ local swim_member_event_mt = {
 -- @return A function to set as a trigger.
 --
 local function swim_on_member_event_new(s, callback, ctx)
+    -- Do not keep a hard reference to a SWIM instance. Otherwise
+    -- it is a cyclic reference, and both the instance and the
+    -- trigger will never be GC-ed.
+    s = setmetatable({s}, {__mode = 'v'})
     return function(member_ptr, event_mask)
-        local m = swim_wrap_member(s, member_ptr)
-        local event = setmetatable({event_mask}, swim_member_event_mt)
-        return callback(m, event, ctx)
+        local s = s[1]
+        if s then
+            local m = swim_wrap_member(s, member_ptr)
+            local event = setmetatable({event_mask}, swim_member_event_mt)
+            return callback(m, event, ctx)
+        end
     end
 end
 
diff --git a/test/swim/swim.result b/test/swim/swim.result
index 05a108614..6004971d0 100644
--- a/test/swim/swim.result
+++ b/test/swim/swim.result
@@ -1574,6 +1574,27 @@ s1:delete()
 s2:delete()
 ---
 ...
+--
+-- Check that Lua triggers don't keep a reference of SWIM instance
+-- preventing its GC.
+--
+s = swim.new({uri = 0, uuid = uuid(1)})
+---
+...
+_ = s:on_member_event(function() end)
+---
+...
+s = setmetatable({s}, {__mode = 'v'})
+---
+...
+collectgarbage('collect')
+---
+- 0
+...
+s
+---
+- []
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/swim/swim.test.lua b/test/swim/swim.test.lua
index b4e4adafc..f1139087c 100644
--- a/test/swim/swim.test.lua
+++ b/test/swim/swim.test.lua
@@ -535,4 +535,14 @@ s1:cfg({generation = 5})
 s1:delete()
 s2:delete()
 
+--
+-- Check that Lua triggers don't keep a reference of SWIM instance
+-- preventing its GC.
+--
+s = swim.new({uri = 0, uuid = uuid(1)})
+_ = s:on_member_event(function() end)
+s = setmetatable({s}, {__mode = 'v'})
+collectgarbage('collect')
+s
+
 test_run:cmd("clear filter")
-- 
2.20.1 (Apple Git-117)

[tarantool-patches] [PATCH 2/4] swim: fix a dangerous yield in ffi.gc

[Vladislav Shpilevoy]

FFI can't survive yields. A yield in ffi.C.func() leads to a
crash; yield in ffi.gc is not documented as allowed. Yield in any
GC function leads to garbage collector stuck until the yield is
finished.

This patch makes SWIM GC callback non-yielding. Now yielding
swim_delete() is called in a separate fiber created in GC
callback, but started at the end of event loop only.

Follow up #3234
---
 src/lua/swim.lua        | 18 +++++++++++++++++-
 test/swim/swim.result   | 30 ++++++++++++++++++++++++++++++
 test/swim/swim.test.lua | 12 ++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index bae6c0da6..686c376cc 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -3,6 +3,7 @@ local uuid = require('uuid')
 local buffer = require('buffer')
 local msgpack = require('msgpack')
 local crypto = require('crypto')
+local fiber = require('fiber')
 local internal = require('swim')
 
 ffi.cdef[[
@@ -948,6 +949,21 @@ swim_cfg_not_configured_mt.__call = swim_cfg_first_call
 -- removed members erasure - GC drops them automatically.
 local cache_table_mt = { __mode = 'v' }
 
+--
+-- SWIM garbage collection function. It can't delete the SWIM
+-- instance immediately, because it is invoked by Lua GC. Firstly,
+-- it is not safe to yield in FFI - Jit can't survive a yield.
+-- Secondly, it is not safe to yield in any GC function, because
+-- it stops garbage collection. Instead, here a new fiber is
+-- created without yields, which works at the end of the event
+-- loop, and deletes the instance asynchronously.
+--
+local function swim_gc(ptr)
+    fiber.new(function()
+        internal.swim_delete(ptr)
+    end)
+end
+
 --
 -- Create a new SWIM instance, and configure if @a cfg is
 -- provided.
@@ -969,7 +985,7 @@ local function swim_new(cfg)
     if ptr == nil then
         return nil, box.error.last()
     end
-    ffi.gc(ptr, internal.swim_delete)
+    ffi.gc(ptr, swim_gc)
     local s = setmetatable({
         ptr = ptr,
         cfg = setmetatable({index = {}}, swim_cfg_not_configured_mt),
diff --git a/test/swim/swim.result b/test/swim/swim.result
index 6004971d0..318b4bed9 100644
--- a/test/swim/swim.result
+++ b/test/swim/swim.result
@@ -1595,6 +1595,36 @@ s
 ---
 - []
 ...
+--
+-- Check that SWIM GC doesn't block nor crash garbage collector.
+--
+s = swim.new()
+---
+...
+allow_gc = false
+---
+...
+_ = s:on_member_event(function() while not allow_gc do pcall(fiber.sleep, 0.01) end end)
+---
+...
+s:cfg({uri = 0, uuid = uuid(1)})
+---
+- true
+...
+s = setmetatable({s}, {__mode = 'v'})
+---
+...
+collectgarbage('collect')
+---
+- 0
+...
+s
+---
+- []
+...
+allow_gc = true
+---
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/swim/swim.test.lua b/test/swim/swim.test.lua
index f1139087c..16b77b602 100644
--- a/test/swim/swim.test.lua
+++ b/test/swim/swim.test.lua
@@ -545,4 +545,16 @@ s = setmetatable({s}, {__mode = 'v'})
 collectgarbage('collect')
 s
 
+--
+-- Check that SWIM GC doesn't block nor crash garbage collector.
+--
+s = swim.new()
+allow_gc = false
+_ = s:on_member_event(function() while not allow_gc do pcall(fiber.sleep, 0.01) end end)
+s:cfg({uri = 0, uuid = uuid(1)})
+s = setmetatable({s}, {__mode = 'v'})
+collectgarbage('collect')
+s
+allow_gc = true
+
 test_run:cmd("clear filter")
-- 
2.20.1 (Apple Git-117)

[tarantool-patches] [PATCH 3/4] swim: fix inability to set generation only

[Vladislav Shpilevoy]

swim.new() is declared as allowed to be called before swim:cfg().
But in fact swim.new({generation = ...}) didn't work because
after generation extraction the empty config {} was passed to
swim:cfg() and led to an error.

The patch allows to call swim.new() with generation only, as well
as without parameters at all.

Follow up #4280
---
 src/lua/swim.lua        | 5 +++++
 test/swim/swim.result   | 6 ++++++
 test/swim/swim.test.lua | 4 ++++
 3 files changed, 15 insertions(+)

diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index 686c376cc..69d077c37 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -980,6 +980,11 @@ local function swim_new(cfg)
         -- Nullify in order to do not raise errors in the
         -- following swim:cfg() about unknown parameters.
         cfg.generation = nil
+        -- When only 'generation' is specified, empty config will
+        -- raise an error at swim:cfg(). It should not.
+        if next(cfg) == nil then
+            cfg = nil
+        end
     end
     local ptr = internal.swim_new(generation)
     if ptr == nil then
diff --git a/test/swim/swim.result b/test/swim/swim.result
index 318b4bed9..26f74f9f4 100644
--- a/test/swim/swim.result
+++ b/test/swim/swim.result
@@ -1574,6 +1574,12 @@ s1:delete()
 s2:delete()
 ---
 ...
+-- It is allowed to create a SWIM with a generation value, but do
+-- not configure it.
+swim.new({generation = 0})
+---
+- []
+...
 --
 -- Check that Lua triggers don't keep a reference of SWIM instance
 -- preventing its GC.
diff --git a/test/swim/swim.test.lua b/test/swim/swim.test.lua
index 16b77b602..496beced5 100644
--- a/test/swim/swim.test.lua
+++ b/test/swim/swim.test.lua
@@ -535,6 +535,10 @@ s1:cfg({generation = 5})
 s1:delete()
 s2:delete()
 
+-- It is allowed to create a SWIM with a generation value, but do
+-- not configure it.
+swim.new({generation = 0})
+
 --
 -- Check that Lua triggers don't keep a reference of SWIM instance
 -- preventing its GC.
-- 
2.20.1 (Apple Git-117)

[tarantool-patches] [PATCH 4/4] swim: default generation is timestamp

[Vladislav Shpilevoy]

Generation is supposed to be a persistent counter to distinguish
between different installations of the same SWIM instance. By
default it was set to 0, which was quite unsafe.

Kostja proposed an easy and bright solution - generation could be
set to timestamp by default. In such a case on each restart it is
almost 100% will be different.

Follow up #4280
---
 src/lua/swim.lua        |  4 +++-
 test/swim/swim.result   | 24 ++++++++++++------------
 test/swim/swim.test.lua | 24 ++++++++++++------------
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index 69d077c37..01eeb7eae 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -969,7 +969,7 @@ end
 -- provided.
 --
 local function swim_new(cfg)
-    local generation = 0
+    local generation
     if cfg and type(cfg) == 'table' and cfg.generation then
         generation = cfg.generation
         if type(generation) ~= 'number' or generation < 0 or
@@ -985,6 +985,8 @@ local function swim_new(cfg)
         if next(cfg) == nil then
             cfg = nil
         end
+    else
+        generation = fiber.time64()
     end
     local ptr = internal.swim_new(generation)
     if ptr == nil then
diff --git a/test/swim/swim.result b/test/swim/swim.result
index 26f74f9f4..c7d539f5d 100644
--- a/test/swim/swim.result
+++ b/test/swim/swim.result
@@ -208,10 +208,10 @@ s:delete()
 --
 -- Basic member table manipulations.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01})
+s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 ---
 ...
-s2 = swim.new({uuid = uuid(2), uri = listen_uri, heartbeat_rate = 0.01})
+s2 = swim.new({uuid = uuid(2), uri = listen_uri, heartbeat_rate = 0.01, generation = 0})
 ---
 ...
 s1.broadcast()
@@ -381,7 +381,7 @@ s2:delete()
 --
 -- Member API.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri()})
+s1 = swim.new({uuid = uuid(1), uri = uri(), generation = 0})
 ---
 ...
 s = s1:self()
@@ -699,10 +699,10 @@ self:is_dropped()
 --
 -- Check payload dissemination.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01})
+s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 ---
 ...
-s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01})
+s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 ---
 ...
 s1:add_member({uuid = uuid(2), uri = s2:self():uri()})
@@ -757,7 +757,7 @@ s2:delete()
 function iterate() local t = {} for k, v in s:pairs() do table.insert(t, {k, v}) end return t end
 ---
 ...
-s = swim.new()
+s = swim.new({generation = 0})
 ---
 ...
 iterate()
@@ -832,10 +832,10 @@ s:delete()
 --
 -- Payload caching.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01})
+s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 ---
 ...
-s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01})
+s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 ---
 ...
 s1_self = s1:self()
@@ -1014,10 +1014,10 @@ s:delete()
 --
 -- Encryption.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri()})
+s1 = swim.new({uuid = uuid(1), uri = uri(), generation = 0})
 ---
 ...
-s2 = swim.new({uuid = uuid(2), uri = uri()})
+s2 = swim.new({uuid = uuid(2), uri = uri(), generation = 0})
 ---
 ...
 s1.set_codec()
@@ -1224,7 +1224,7 @@ s2:delete()
 -- gh-4250: allow to set triggers on a new member appearance, old
 -- member drop, member update.
 --
-s1 = swim.new()
+s1 = swim.new({generation = 0})
 ---
 ...
 s1.on_member_event()
@@ -1313,7 +1313,7 @@ t_yield_id = s1:on_member_event(t_yield, 'ctx2')
 f_need_sleep = true
 ---
 ...
-s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01})
+s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 ---
 ...
 s2:add_member({uuid = s1:self():uuid(), uri = s1:self():uri()})
diff --git a/test/swim/swim.test.lua b/test/swim/swim.test.lua
index 496beced5..c949b3be2 100644
--- a/test/swim/swim.test.lua
+++ b/test/swim/swim.test.lua
@@ -71,8 +71,8 @@ s:delete()
 --
 -- Basic member table manipulations.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01})
-s2 = swim.new({uuid = uuid(2), uri = listen_uri, heartbeat_rate = 0.01})
+s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01, generation = 0})
+s2 = swim.new({uuid = uuid(2), uri = listen_uri, heartbeat_rate = 0.01, generation = 0})
 
 s1.broadcast()
 s1:broadcast('wrong port')
@@ -132,7 +132,7 @@ s2:delete()
 --
 -- Member API.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri()})
+s1 = swim.new({uuid = uuid(1), uri = uri(), generation = 0})
 s = s1:self()
 s
 s:status()
@@ -233,8 +233,8 @@ self:is_dropped()
 --
 -- Check payload dissemination.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01})
-s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01})
+s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01, generation = 0})
+s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 s1:add_member({uuid = uuid(2), uri = s2:self():uri()})
 while s2:size() ~= 2 do fiber.sleep(0.01) end
 s1_view = s2:member_by_uuid(uuid(1))
@@ -256,7 +256,7 @@ s2:delete()
 -- Iterators.
 --
 function iterate() local t = {} for k, v in s:pairs() do table.insert(t, {k, v}) end return t end
-s = swim.new()
+s = swim.new({generation = 0})
 iterate()
 s:cfg({uuid = uuid(1), uri = uri(), gc_mode = 'off'})
 s.pairs()
@@ -270,8 +270,8 @@ s:delete()
 --
 -- Payload caching.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01})
-s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01})
+s1 = swim.new({uuid = uuid(1), uri = uri(), heartbeat_rate = 0.01, generation = 0})
+s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 s1_self = s1:self()
 _ = s1:add_member({uuid = s2:self():uuid(), uri = s2:self():uri()})
 _ = s2:add_member({uuid = s1_self:uuid(), uri = s1_self:uri()})
@@ -344,8 +344,8 @@ s:delete()
 --
 -- Encryption.
 --
-s1 = swim.new({uuid = uuid(1), uri = uri()})
-s2 = swim.new({uuid = uuid(2), uri = uri()})
+s1 = swim.new({uuid = uuid(1), uri = uri(), generation = 0})
+s2 = swim.new({uuid = uuid(2), uri = uri(), generation = 0})
 
 s1.set_codec()
 s1:set_codec(100)
@@ -418,7 +418,7 @@ s2:delete()
 -- gh-4250: allow to set triggers on a new member appearance, old
 -- member drop, member update.
 --
-s1 = swim.new()
+s1 = swim.new({generation = 0})
 s1.on_member_event()
 
 m_list = {}
@@ -452,7 +452,7 @@ m_list = {} e_list = {} ctx_list = {}
 
 t_yield_id = s1:on_member_event(t_yield, 'ctx2')
 f_need_sleep = true
-s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01})
+s2 = swim.new({uuid = uuid(2), uri = uri(), heartbeat_rate = 0.01, generation = 0})
 s2:add_member({uuid = s1:self():uuid(), uri = s1:self():uri()})
 while s1:size() ~= 2 do fiber.sleep(0.01) end
 -- Only first trigger worked. Second is waiting, because first
-- 
2.20.1 (Apple Git-117)

[tarantool-patches] Re: [PATCH 0/4] SWIM amendments

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/06/28 09:43]:
> A set of SWIM follow ups related to a couple of tickets. Most of them are
> bugfixes.

lgtm to all patches in the stack

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/4] SWIM amendments

[Vladislav Shpilevoy]

Pushed to the master.

On 28/06/2019 13:25, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/06/28 09:43]:
>> A set of SWIM follow ups related to a couple of tickets. Most of them are
>> bugfixes.
> 
> lgtm to all patches in the stack
>