Skip to content

Commit

Permalink
swim: fix a dangerous yield in ffi.gc
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Gerold103 committed Jun 27, 2019
1 parent 8570762 commit 0b16f1d
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 1 deletion.
18 changes: 17 additions & 1 deletion src/lua/swim.lua
Expand Up @@ -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[[
Expand Down Expand Up @@ -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.
Expand All @@ -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),
Expand Down
30 changes: 30 additions & 0 deletions test/swim/swim.result
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions test/swim/swim.test.lua
Expand Up @@ -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")

0 comments on commit 0b16f1d

Please sign in to comment.