[Tarantool-patches] [PATCH v2 0/3] fio: close unused descriptors automatically

[Tarantool-patches] [PATCH v2 0/3] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

The patchset introduces a fiber worker singleton object and uses
it for garbage collection of fio and SWIM objects. This is a
workaround for inability to yield in ffi.gc hook, which is used by
fio and SWIM.

SWIM had GC before, but it was creating a new fiber for each GC.
Fio didn't have GC.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4727-fio-gc
Issue: https://github.com/tarantool/tarantool/issues/4727

@ChangeLog
- fio descriptors are closed on garbage collection (gh-4727).

Changes in V2:
- Added a special single fiber for garbage collection of objects,
  which yield in their destructors.

Vladislav Shpilevoy (3):
  fiber: introduce schedule_task() internal function
  fio: close unused descriptors automatically
  swim: use fiber._internal.schedule_task() for GC

 src/lua/fiber.c                  |  15 +++++
 src/lua/fiber.lua                |  60 ++++++++++++++++++
 src/lua/fio.lua                  |  34 ++++++++--
 src/lua/swim.lua                 |  11 ++--
 test/app/fiber.result            |  72 +++++++++++++++++++++
 test/app/fiber.test.lua          |  41 ++++++++++++
 test/app/gh-4727-fio-gc.result   | 104 +++++++++++++++++++++++++++++++
 test/app/gh-4727-fio-gc.test.lua |  60 ++++++++++++++++++
 8 files changed, 385 insertions(+), 12 deletions(-)
 create mode 100644 test/app/gh-4727-fio-gc.result
 create mode 100644 test/app/gh-4727-fio-gc.test.lua

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function

[Vladislav Shpilevoy]

fiber._internal.schedule_task() is an API for a singleton fiber
worker object. It serves for not urgent delayed execution of
functions. Main purpose - schedule execution of a function, which
is going to yield, from a context, where a yield is not allowed.
Such as an FFI object's GC callback.

It will be used by SWIM and by fio, whose destruction yields, but
they need to use ffi.gc hook, where a yield is not allowed.

Part of #4727
---
 src/lua/fiber.c         | 15 +++++++++
 src/lua/fiber.lua       | 60 ++++++++++++++++++++++++++++++++++
 test/app/fiber.result   | 72 +++++++++++++++++++++++++++++++++++++++++
 test/app/fiber.test.lua | 41 +++++++++++++++++++++++
 4 files changed, 188 insertions(+)

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 575a020d0..ddf827ab6 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -828,6 +828,19 @@ lbox_fiber_set_joinable(struct lua_State *L)
 	return 0;
 }
 
+/**
+ * Alternative to fiber.sleep(infinite) which does not participate
+ * in an event loop at all until an explicit wakeup. This is less
+ * overhead. Useful for fibers sleeping most of the time.
+ */
+static int
+lbox_fiber_sleep_infinite(struct lua_State *L)
+{
+	(void) L;
+	fiber_yield();
+	return 0;
+}
+
 static const struct luaL_Reg lbox_fiber_meta [] = {
 	{"id", lbox_fiber_id},
 	{"name", lbox_fiber_name},
@@ -865,6 +878,8 @@ static const struct luaL_Reg fiberlib[] = {
 	{"new", lbox_fiber_new},
 	{"status", lbox_fiber_status},
 	{"name", lbox_fiber_name},
+	/* Internal functions, to hide in fiber.lua. */
+	{"sleep_infinite", lbox_fiber_sleep_infinite},
 	{NULL, NULL}
 };
 
diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
index 8712ee0d6..d0b765b60 100644
--- a/src/lua/fiber.lua
+++ b/src/lua/fiber.lua
@@ -34,4 +34,64 @@ fiber.time = fiber_time
 fiber.time64 = fiber_time64
 fiber.clock = fiber_clock
 fiber.clock64 = fiber_clock64
+
+local sleep_infinite = fiber.sleep_infinite
+fiber.sleep_infinite = nil
+
+local worker_next_task = nil
+local worker_last_task = nil
+local worker_fiber = nil
+
+--
+-- Worker is a singleton fiber for not urgent delayed execution of
+-- functions. Main purpose - schedule execution of a function,
+-- which is going to yield, from a context, where a yield is not
+-- allowed. Such as an FFI object's GC callback.
+--
+local function worker_f()
+    local task
+    while true do
+        while true do
+            task = worker_next_task
+            if task then
+                break
+            end
+            sleep_infinite()
+        end
+        worker_next_task = task.next
+        task.f(task.arg)
+        fiber.sleep(0)
+    end
+end
+
+local function worker_safe_f()
+    pcall(worker_f)
+    -- This fiber is probably canceled and now is not able to
+    -- sleep, create a new one.
+    worker_fiber = fiber.new(worker_safe_f)
+end
+
+worker_fiber = fiber.new(worker_safe_f)
+
+local function worker_schedule_task(f, arg)
+    local task = {f = f, arg = arg}
+    if not worker_next_task then
+        worker_next_task = task
+    else
+        worker_last_task.next = task
+    end
+    worker_last_task = task
+    worker_fiber:wakeup()
+end
+
+-- Start from '_' to hide it from auto completion.
+fiber._internal = fiber._internal or {}
+fiber._internal.schedule_task = worker_schedule_task
+
+setmetatable(fiber, {__serialize = function(self)
+    local res = table.copy(self)
+    res._internal = nil
+    return setmetatable(res, {})
+end})
+
 return fiber
diff --git a/test/app/fiber.result b/test/app/fiber.result
index 6d9604ad8..bd60e1483 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1561,6 +1561,78 @@ fiber.top()
 ---
 - error: fiber.top() is disabled. Enable it with fiber.top_enable() first
 ...
+--
+-- fiber._internal.schedule_task() - API for internal usage for
+-- delayed execution of a function.
+--
+glob_arg = {}
+---
+...
+count = 0
+---
+...
+function task_f(arg)                                                            \
+    count = count + 1                                                           \
+    table.insert(glob_arg, arg)                                                 \
+    arg = arg + 1                                                               \
+    if arg <= 3 then                                                            \
+        fiber._internal.schedule_task(task_f, arg)                              \
+    else                                                                        \
+        fiber.self():cancel()                                                   \
+        error('Worker is broken')                                               \
+    end                                                                         \
+end
+---
+...
+for i = 1, 3 do                                                                 \
+    local csw1 = fiber.info()[fiber.id()].csw                                   \
+    fiber._internal.schedule_task(task_f, i)                                    \
+    local csw2 = fiber.info()[fiber.id()].csw                                   \
+    assert(csw1 == csw2 and csw1 ~= nil)                                        \
+end
+---
+...
+old_count = count
+---
+...
+test_run:wait_cond(function()                                                   \
+    fiber.yield()                                                               \
+    if count == old_count then                                                  \
+        return true                                                             \
+    end                                                                         \
+    old_count = count                                                           \
+end)
+---
+- true
+...
+glob_arg
+---
+- - 1
+  - 2
+  - 3
+  - 2
+  - 3
+  - 3
+...
+count
+---
+- 6
+...
+-- Ensure, that after all tasks are finished, the worker didn't
+-- stuck somewhere.
+glob_arg = nil
+---
+...
+fiber._internal.schedule_task(function(arg) glob_arg = arg end, 100)
+---
+...
+fiber.yield()
+---
+...
+glob_arg
+---
+- 100
+...
 -- cleanup
 test_run:cmd("clear filter")
 ---
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index 6df210d9c..f10782d1f 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -688,6 +688,47 @@ tbl.time > 0
 fiber.top_disable()
 fiber.top()
 
+--
+-- fiber._internal.schedule_task() - API for internal usage for
+-- delayed execution of a function.
+--
+glob_arg = {}
+count = 0
+function task_f(arg)                                                            \
+    count = count + 1                                                           \
+    table.insert(glob_arg, arg)                                                 \
+    arg = arg + 1                                                               \
+    if arg <= 3 then                                                            \
+        fiber._internal.schedule_task(task_f, arg)                              \
+    else                                                                        \
+        fiber.self():cancel()                                                   \
+        error('Worker is broken')                                               \
+    end                                                                         \
+end
+for i = 1, 3 do                                                                 \
+    local csw1 = fiber.info()[fiber.id()].csw                                   \
+    fiber._internal.schedule_task(task_f, i)                                    \
+    local csw2 = fiber.info()[fiber.id()].csw                                   \
+    assert(csw1 == csw2 and csw1 ~= nil)                                        \
+end
+old_count = count
+test_run:wait_cond(function()                                                   \
+    fiber.yield()                                                               \
+    if count == old_count then                                                  \
+        return true                                                             \
+    end                                                                         \
+    old_count = count                                                           \
+end)
+glob_arg
+count
+
+-- Ensure, that after all tasks are finished, the worker didn't
+-- stuck somewhere.
+glob_arg = nil
+fiber._internal.schedule_task(function(arg) glob_arg = arg end, 100)
+fiber.yield()
+glob_arg
+
 -- cleanup
 test_run:cmd("clear filter")
 
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

Fio.open() returned a file descriptor, which was not closed
automatically after all its links were nullified. In other words,
GC didn't close the descriptor.

This was not really useful, because after fio.open() an exception
may appear, and user needed to workaround this to manually call
fio_object:close(). Also this was not consistent with io.open().

Now fio.open() object closes the descriptor automatically when
GCed.

Closes #4727

@TarantoolBot document
Title: fio descriptor is closed automatically by GC

fio.open() returns a descriptor which can be closed manually by
calling :close() method, or it will be closed automatically, when
it has no references, and GC deletes it.

:close() method existed always, auto GC was added just now.
---
 src/lua/fio.lua                  |  34 ++++++++--
 test/app/gh-4727-fio-gc.result   | 104 +++++++++++++++++++++++++++++++
 test/app/gh-4727-fio-gc.test.lua |  60 ++++++++++++++++++
 3 files changed, 192 insertions(+), 6 deletions(-)
 create mode 100644 test/app/gh-4727-fio-gc.result
 create mode 100644 test/app/gh-4727-fio-gc.test.lua

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 4692e1026..1ffb2ebfd 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -5,6 +5,7 @@ local ffi = require('ffi')
 local buffer = require('buffer')
 local fiber = require('fiber')
 local errno = require('errno')
+local schedule_task = fiber._internal.schedule_task
 
 ffi.cdef[[
     int umask(int mask);
@@ -141,10 +142,12 @@ end
 
 fio_methods.close = function(self)
     local res, err = internal.close(self.fh)
-    self.fh = -1
     if err ~= nil then
         return false, err
     end
+    ffi.gc(self._gc, nil)
+    self._gc = nil
+    self.fh = -1
     return res
 end
 
@@ -160,7 +163,23 @@ fio_methods.stat = function(self)
     return internal.fstat(self.fh)
 end
 
-local fio_mt = { __index = fio_methods }
+local fio_mt = {
+    __index = fio_methods,
+    __serialize = function(obj)
+        return {fh = obj.fh}
+    end,
+}
+
+local function fio_wrap(fh)
+    return setmetatable({
+        fh = fh,
+        _gc = ffi.gc(ffi.new('char[1]'), function()
+            -- FFI GC can't yield. Internal.close() yields.
+            -- Collect the garbage later, in a worker fiber.
+            schedule_task(internal.close, fh)
+        end)
+    }, fio_mt)
+end
 
 fio.open = function(path, flags, mode)
     local iflag = 0
@@ -202,10 +221,13 @@ fio.open = function(path, flags, mode)
     if err ~= nil then
         return nil, err
     end
-
-    fh = { fh = fh }
-    setmetatable(fh, fio_mt)
-    return fh
+    local ok, res = pcall(fio_wrap, fh)
+    if not ok then
+        internal.close(fh)
+        -- This is either OOM or bad syntax, both require throw.
+        return error(res)
+    end
+    return res
 end
 
 fio.pathjoin = function(...)
diff --git a/test/app/gh-4727-fio-gc.result b/test/app/gh-4727-fio-gc.result
new file mode 100644
index 000000000..d793eded3
--- /dev/null
+++ b/test/app/gh-4727-fio-gc.result
@@ -0,0 +1,104 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+fio = require('fio')
+ | ---
+ | ...
+--
+-- gh-4727: fio handler GC.
+--
+flags = {'O_CREAT', 'O_RDWR'}
+ | ---
+ | ...
+mode = {'S_IRWXU'}
+ | ---
+ | ...
+filename = 'test4727.txt'
+ | ---
+ | ...
+fh1 = nil
+ | ---
+ | ...
+fh2 = nil
+ | ---
+ | ...
+-- Idea of the test is that according to the Open Group standard,
+-- open() always returns the smallest available descriptor. This
+-- means, that in 'open() + close() + open()' the second open()
+-- should return the same value as the first call, if no other
+-- threads/fibers managed to interfere. Because of the
+-- interference the sequence may need to be called multiple times
+-- to catch a couple of equal descriptors.
+test_run:wait_cond(function()                                       \
+    collectgarbage('collect')                                       \
+    local f = fio.open(filename, flags, mode)                       \
+    fh1 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+-- GC function of a fio object works in a separate fiber. Give it   \
+-- time to execute.                                                 \
+    fiber.yield()                                                   \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+    fiber.yield()                                                   \
+    return fh1 == fh2                                               \
+end) or {fh1, fh2}
+ | ---
+ | - true
+ | ...
+
+-- Ensure, that GC does not break anything after explicit close.
+-- Idea of the test is the same as in the previous test, but now
+-- the second descriptor is used for something. If GC of the first
+-- fio object is called even after close(), it would close the
+-- same descriptor, already used by the second fio object. And it
+-- won't be able to write anything. Or will write, but to a
+-- totally different descriptor created by some other
+-- fiber/thread. This is why read() is called on the same file
+-- afterwards.
+f = nil
+ | ---
+ | ...
+test_run:wait_cond(function()                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh1 = f.fh                                                      \
+    f:close()                                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    return fh1 == fh2                                               \
+end)
+ | ---
+ | - true
+ | ...
+collectgarbage('collect')
+ | ---
+ | - 0
+ | ...
+fiber.yield()
+ | ---
+ | ...
+f:write('test')
+ | ---
+ | - true
+ | ...
+f:close()
+ | ---
+ | - true
+ | ...
+f = fio.open(filename, flags, mode)
+ | ---
+ | ...
+f:read()
+ | ---
+ | - test
+ | ...
+f:close()
+ | ---
+ | - true
+ | ...
diff --git a/test/app/gh-4727-fio-gc.test.lua b/test/app/gh-4727-fio-gc.test.lua
new file mode 100644
index 000000000..d0ab585ca
--- /dev/null
+++ b/test/app/gh-4727-fio-gc.test.lua
@@ -0,0 +1,60 @@
+test_run = require('test_run').new()
+fiber = require('fiber')
+fio = require('fio')
+--
+-- gh-4727: fio handler GC.
+--
+flags = {'O_CREAT', 'O_RDWR'}
+mode = {'S_IRWXU'}
+filename = 'test4727.txt'
+fh1 = nil
+fh2 = nil
+-- Idea of the test is that according to the Open Group standard,
+-- open() always returns the smallest available descriptor. This
+-- means, that in 'open() + close() + open()' the second open()
+-- should return the same value as the first call, if no other
+-- threads/fibers managed to interfere. Because of the
+-- interference the sequence may need to be called multiple times
+-- to catch a couple of equal descriptors.
+test_run:wait_cond(function()                                       \
+    collectgarbage('collect')                                       \
+    local f = fio.open(filename, flags, mode)                       \
+    fh1 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+-- GC function of a fio object works in a separate fiber. Give it   \
+-- time to execute.                                                 \
+    fiber.yield()                                                   \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+    fiber.yield()                                                   \
+    return fh1 == fh2                                               \
+end) or {fh1, fh2}
+
+-- Ensure, that GC does not break anything after explicit close.
+-- Idea of the test is the same as in the previous test, but now
+-- the second descriptor is used for something. If GC of the first
+-- fio object is called even after close(), it would close the
+-- same descriptor, already used by the second fio object. And it
+-- won't be able to write anything. Or will write, but to a
+-- totally different descriptor created by some other
+-- fiber/thread. This is why read() is called on the same file
+-- afterwards.
+f = nil
+test_run:wait_cond(function()                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh1 = f.fh                                                      \
+    f:close()                                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    return fh1 == fh2                                               \
+end)
+collectgarbage('collect')
+fiber.yield()
+f:write('test')
+f:close()
+f = fio.open(filename, flags, mode)
+f:read()
+f:close()
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 3/3] swim: use fiber._internal.schedule_task() for GC

[Vladislav Shpilevoy]

swim object created a new fiber in its GC function, because C
function swim_delete() yields, and can't be called from an ffi.gc
hook.

It is not needed since the fiber module has a single worker
exactly for such cases. The patch uses it.

Follow up #4727
---
 src/lua/swim.lua | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/lua/swim.lua b/src/lua/swim.lua
index 01eeb7eae..0859915c9 100644
--- a/src/lua/swim.lua
+++ b/src/lua/swim.lua
@@ -5,6 +5,7 @@ local msgpack = require('msgpack')
 local crypto = require('crypto')
 local fiber = require('fiber')
 local internal = require('swim')
+local schedule_task = fiber._internal.schedule_task
 
 ffi.cdef[[
     struct swim;
@@ -954,14 +955,12 @@ local cache_table_mt = { __mode = 'v' }
 -- 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.
+-- it stops garbage collection. Instead, here GC is delayed, 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)
+    schedule_task(internal.swim_delete, ptr)
 end
 
 --
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function

[Igor Munkin]

Vlad,

Thanks for the patch! Please consider several minor comments I left.

On 03.03.20, Vladislav Shpilevoy wrote:
> fiber._internal.schedule_task() is an API for a singleton fiber
> worker object. It serves for not urgent delayed execution of
> functions. Main purpose - schedule execution of a function, which
> is going to yield, from a context, where a yield is not allowed.
> Such as an FFI object's GC callback.
> 
> It will be used by SWIM and by fio, whose destruction yields, but
> they need to use ffi.gc hook, where a yield is not allowed.

Minor: AFAIR __gc metamethod[1] also obligues the one to avoid
coroutine/fiber switch[2]. Thereby GC finalizer looks to be more
accurate term here.

> 
> Part of #4727
> ---
>  src/lua/fiber.c         | 15 +++++++++
>  src/lua/fiber.lua       | 60 ++++++++++++++++++++++++++++++++++
>  test/app/fiber.result   | 72 +++++++++++++++++++++++++++++++++++++++++
>  test/app/fiber.test.lua | 41 +++++++++++++++++++++++
>  4 files changed, 188 insertions(+)
> 
> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 575a020d0..ddf827ab6 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -828,6 +828,19 @@ lbox_fiber_set_joinable(struct lua_State *L)
>  	return 0;
>  }
>  
> +/**
> + * Alternative to fiber.sleep(infinite) which does not participate
> + * in an event loop at all until an explicit wakeup. This is less
> + * overhead. Useful for fibers sleeping most of the time.
> + */

Minor: I guess <lbox_fiber_stall> and <fiber.stall> are good names for
this API. Feel free to ignore.

> +static int
> +lbox_fiber_sleep_infinite(struct lua_State *L)
> +{
> +	(void) L;
> +	fiber_yield();
> +	return 0;
> +}
> +
>  static const struct luaL_Reg lbox_fiber_meta [] = {
>  	{"id", lbox_fiber_id},
>  	{"name", lbox_fiber_name},

<snipped>

> diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
> index 8712ee0d6..d0b765b60 100644
> --- a/src/lua/fiber.lua
> +++ b/src/lua/fiber.lua
> @@ -34,4 +34,64 @@ fiber.time = fiber_time
>  fiber.time64 = fiber_time64
>  fiber.clock = fiber_clock
>  fiber.clock64 = fiber_clock64
> +
> +local sleep_infinite = fiber.sleep_infinite
> +fiber.sleep_infinite = nil
> +
> +local worker_next_task = nil
> +local worker_last_task = nil
> +local worker_fiber = nil
> +
> +--
> +-- Worker is a singleton fiber for not urgent delayed execution of
> +-- functions. Main purpose - schedule execution of a function,
> +-- which is going to yield, from a context, where a yield is not
> +-- allowed. Such as an FFI object's GC callback.
> +--
> +local function worker_f()
> +    local task
> +    while true do
> +        while true do
> +            task = worker_next_task
> +            if task then
> +                break
> +            end
> +            sleep_infinite()
> +        end
> +        worker_next_task = task.next
> +        task.f(task.arg)
> +        fiber.sleep(0)
> +    end
> +end
> +
> +local function worker_safe_f()
> +    pcall(worker_f)
> +    -- This fiber is probably canceled and now is not able to
> +    -- sleep, create a new one.

Minor: I guess it worth mentioning the fact that <worker_f> doesn't
return. It would definitely make the comment clearer.

> +    worker_fiber = fiber.new(worker_safe_f)
> +end
> +
> +worker_fiber = fiber.new(worker_safe_f)
> +
> +local function worker_schedule_task(f, arg)
> +    local task = {f = f, arg = arg}
> +    if not worker_next_task then
> +        worker_next_task = task
> +    else

Minor: It would be great to add an assert here, since nothing except the
current implementation prevents us from indexing a nil value. Feel free
to ignore.

> +        worker_last_task.next = task
> +    end
> +    worker_last_task = task
> +    worker_fiber:wakeup()
> +end
> +
> +-- Start from '_' to hide it from auto completion.
> +fiber._internal = fiber._internal or {}
> +fiber._internal.schedule_task = worker_schedule_task
> +
> +setmetatable(fiber, {__serialize = function(self)
> +    local res = table.copy(self)
> +    res._internal = nil
> +    return setmetatable(res, {})
> +end})
> +
>  return fiber

<snipped>

> diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
> index 6df210d9c..f10782d1f 100644
> --- a/test/app/fiber.test.lua
> +++ b/test/app/fiber.test.lua
> @@ -688,6 +688,47 @@ tbl.time > 0
>  fiber.top_disable()
>  fiber.top()
>  
> +--
> +-- fiber._internal.schedule_task() - API for internal usage for
> +-- delayed execution of a function.
> +--
> +glob_arg = {}
> +count = 0
> +function task_f(arg)                                                            \
> +    count = count + 1                                                           \
> +    table.insert(glob_arg, arg)                                                 \
> +    arg = arg + 1                                                               \
> +    if arg <= 3 then                                                            \
> +        fiber._internal.schedule_task(task_f, arg)                              \
> +    else                                                                        \
> +        fiber.self():cancel()                                                   \
> +        error('Worker is broken')                                               \
> +    end                                                                         \
> +end
> +for i = 1, 3 do                                                                 \
> +    local csw1 = fiber.info()[fiber.id()].csw                                   \
> +    fiber._internal.schedule_task(task_f, i)                                    \
> +    local csw2 = fiber.info()[fiber.id()].csw                                   \
> +    assert(csw1 == csw2 and csw1 ~= nil)                                        \

Minor: IMHO it's better to check that csw1 is a 'number'. Feel free to
ignore.

> +end
> +old_count = count
> +test_run:wait_cond(function()                                                   \
> +    fiber.yield()                                                               \
> +    if count == old_count then                                                  \
> +        return true                                                             \
> +    end                                                                         \
> +    old_count = count                                                           \
> +end)
> +glob_arg
> +count
> +
> +-- Ensure, that after all tasks are finished, the worker didn't
> +-- stuck somewhere.
> +glob_arg = nil
> +fiber._internal.schedule_task(function(arg) glob_arg = arg end, 100)
> +fiber.yield()
> +glob_arg
> +
>  -- cleanup
>  test_run:cmd("clear filter")
>  
> -- 
> 2.21.1 (Apple Git-122.3)
> 

[1]: https://github.com/tarantool/luajit/blob/tarantool/src/lj_gc.c#L495
[2]: https://github.com/tarantool/luajit/blob/tarantool/src/lj_api.c#L1204

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Igor Munkin]

Vlad,

Thanks for the patch! I see no flaws in it, except the _gc field
presence in fio object. I made another solution based on your cool hacks
with ffi.gc (you can find the diff below). Feel free to disregard my
changes if the problem doesn't bother you.

On 03.03.20, Vladislav Shpilevoy wrote:
> Fio.open() returned a file descriptor, which was not closed
> automatically after all its links were nullified. In other words,
> GC didn't close the descriptor.
> 
> This was not really useful, because after fio.open() an exception
> may appear, and user needed to workaround this to manually call
> fio_object:close(). Also this was not consistent with io.open().
> 
> Now fio.open() object closes the descriptor automatically when
> GCed.
> 
> Closes #4727
> 
> @TarantoolBot document
> Title: fio descriptor is closed automatically by GC
> 
> fio.open() returns a descriptor which can be closed manually by
> calling :close() method, or it will be closed automatically, when
> it has no references, and GC deletes it.
> 
> :close() method existed always, auto GC was added just now.
> ---
>  src/lua/fio.lua                  |  34 ++++++++--
>  test/app/gh-4727-fio-gc.result   | 104 +++++++++++++++++++++++++++++++
>  test/app/gh-4727-fio-gc.test.lua |  60 ++++++++++++++++++
>  3 files changed, 192 insertions(+), 6 deletions(-)
>  create mode 100644 test/app/gh-4727-fio-gc.result
>  create mode 100644 test/app/gh-4727-fio-gc.test.lua
> 
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 4692e1026..1ffb2ebfd 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -5,6 +5,7 @@ local ffi = require('ffi')
>  local buffer = require('buffer')
>  local fiber = require('fiber')
>  local errno = require('errno')
> +local schedule_task = fiber._internal.schedule_task
>  
>  ffi.cdef[[
>      int umask(int mask);
> @@ -141,10 +142,12 @@ end
>  
>  fio_methods.close = function(self)
>      local res, err = internal.close(self.fh)
> -    self.fh = -1
>      if err ~= nil then
>          return false, err
>      end
> +    ffi.gc(self._gc, nil)
> +    self._gc = nil
> +    self.fh = -1
>      return res
>  end
>  
> @@ -160,7 +163,23 @@ fio_methods.stat = function(self)
>      return internal.fstat(self.fh)
>  end
>  
> -local fio_mt = { __index = fio_methods }
> +local fio_mt = {
> +    __index = fio_methods,
> +    __serialize = function(obj)
> +        return {fh = obj.fh}
> +    end,
> +}
> +
> +local function fio_wrap(fh)
> +    return setmetatable({
> +        fh = fh,
> +        _gc = ffi.gc(ffi.new('char[1]'), function()
> +            -- FFI GC can't yield. Internal.close() yields.
> +            -- Collect the garbage later, in a worker fiber.
> +            schedule_task(internal.close, fh)
> +        end)
> +    }, fio_mt)
> +end
>  
>  fio.open = function(path, flags, mode)
>      local iflag = 0
> @@ -202,10 +221,13 @@ fio.open = function(path, flags, mode)
>      if err ~= nil then
>          return nil, err
>      end
> -
> -    fh = { fh = fh }
> -    setmetatable(fh, fio_mt)
> -    return fh
> +    local ok, res = pcall(fio_wrap, fh)
> +    if not ok then
> +        internal.close(fh)
> +        -- This is either OOM or bad syntax, both require throw.
> +        return error(res)
> +    end
> +    return res
>  end
>  
>  fio.pathjoin = function(...)

<snipped>

> -- 
> 2.21.1 (Apple Git-122.3)
> 

================================================================================

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 1ffb2ebfd..595ee175d 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -145,8 +145,7 @@ fio_methods.close = function(self)
     if err ~= nil then
         return false, err
     end
-    ffi.gc(self._gc, nil)
-    self._gc = nil
+    ffi.gc(getmetatable(self).__close, nil)
     self.fh = -1
     return res
 end
@@ -163,22 +162,17 @@ fio_methods.stat = function(self)
     return internal.fstat(self.fh)
 end
 
-local fio_mt = {
-    __index = fio_methods,
-    __serialize = function(obj)
-        return {fh = obj.fh}
-    end,
-}
-
 local function fio_wrap(fh)
     return setmetatable({
         fh = fh,
-        _gc = ffi.gc(ffi.new('char[1]'), function()
+    }, {
+        __index = fio_methods,
+        __close = ffi.gc(ffi.new('char[1]'), function()
             -- FFI GC can't yield. Internal.close() yields.
             -- Collect the garbage later, in a worker fiber.
             schedule_task(internal.close, fh)
         end)
-    }, fio_mt)
+    })
 end
 
 fio.open = function(path, flags, mode)

================================================================================

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 3/3] swim: use fiber._internal.schedule_task() for GC

[Igor Munkin]

Vlad,

Thanks for the patch, LGTM.

On 03.03.20, Vladislav Shpilevoy wrote:
> swim object created a new fiber in its GC function, because C
> function swim_delete() yields, and can't be called from an ffi.gc
> hook.
> 
> It is not needed since the fiber module has a single worker
> exactly for such cases. The patch uses it.
> 
> Follow up #4727
> ---
>  src/lua/swim.lua | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 

<snipped>

> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Igor Munkin]

Vlad,

As you mentioned to me, the doc[1] specify 'userdata' (i.e. opaque type)
as a return type for <fio.open>. Current implementation returns a table,
so as we discussed there is nothing preventing you from dropping all
hacks you used in the patch and implement file handle object via cdata
or userdata.

On 19.03.20, Igor Munkin wrote:
> Vlad,
> 
> Thanks for the patch! I see no flaws in it, except the _gc field
> presence in fio object. I made another solution based on your cool hacks
> with ffi.gc (you can find the diff below). Feel free to disregard my
> changes if the problem doesn't bother you.
> 

<snipped>

> 
> -- 
> Best regards,
> IM

[1]: https://www.tarantool.io/en/doc/2.3/reference/reference_lua/fio/#fio-open

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 19/03/2020 15:52, Igor Munkin wrote:
> Vlad,
> 
> Thanks for the patch! Please consider several minor comments I left.
> 
> On 03.03.20, Vladislav Shpilevoy wrote:
>> fiber._internal.schedule_task() is an API for a singleton fiber
>> worker object. It serves for not urgent delayed execution of
>> functions. Main purpose - schedule execution of a function, which
>> is going to yield, from a context, where a yield is not allowed.
>> Such as an FFI object's GC callback.
>>
>> It will be used by SWIM and by fio, whose destruction yields, but
>> they need to use ffi.gc hook, where a yield is not allowed.
> 
> Minor: AFAIR __gc metamethod[1] also obligues the one to avoid
> coroutine/fiber switch[2]. Thereby GC finalizer looks to be more
> accurate term here.

You know better. Changed 'ffi.gc hook' -> 'GC finalizer'.

>>
>> Part of #4727
>> ---
>>  src/lua/fiber.c         | 15 +++++++++
>>  src/lua/fiber.lua       | 60 ++++++++++++++++++++++++++++++++++
>>  test/app/fiber.result   | 72 +++++++++++++++++++++++++++++++++++++++++
>>  test/app/fiber.test.lua | 41 +++++++++++++++++++++++
>>  4 files changed, 188 insertions(+)
>>
>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index 575a020d0..ddf827ab6 100644
>> --- a/src/lua/fiber.c
>> +++ b/src/lua/fiber.c
>> @@ -828,6 +828,19 @@ lbox_fiber_set_joinable(struct lua_State *L)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * Alternative to fiber.sleep(infinite) which does not participate
>> + * in an event loop at all until an explicit wakeup. This is less
>> + * overhead. Useful for fibers sleeping most of the time.
>> + */
> 
> Minor: I guess <lbox_fiber_stall> and <fiber.stall> are good names for
> this API. Feel free to ignore.

Yeah, sounds much better.

====================
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index ddf827ab6..03b1b959c 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -834,7 +834,7 @@ lbox_fiber_set_joinable(struct lua_State *L)
  * overhead. Useful for fibers sleeping most of the time.
  */
 static int
-lbox_fiber_sleep_infinite(struct lua_State *L)
+lbox_fiber_stall(struct lua_State *L)
 {
 	(void) L;
 	fiber_yield();
@@ -879,7 +879,7 @@ static const struct luaL_Reg fiberlib[] = {
 	{"status", lbox_fiber_status},
 	{"name", lbox_fiber_name},
 	/* Internal functions, to hide in fiber.lua. */
-	{"sleep_infinite", lbox_fiber_sleep_infinite},
+	{"stall", lbox_fiber_stall},
 	{NULL, NULL}
 };
 
diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
index d0b765b60..fa53403d0 100644
--- a/src/lua/fiber.lua
+++ b/src/lua/fiber.lua
@@ -35,8 +35,8 @@ fiber.time64 = fiber_time64
 fiber.clock = fiber_clock
 fiber.clock64 = fiber_clock64
 
-local sleep_infinite = fiber.sleep_infinite
-fiber.sleep_infinite = nil
+local stall = fiber.stall
+fiber.stall = nil
 
 local worker_next_task = nil
 local worker_last_task = nil
@@ -56,7 +56,7 @@ local function worker_f()
             if task then
                 break
             end
-            sleep_infinite()
+            stall()
         end
         worker_next_task = task.next
         task.f(task.arg)
====================

>> diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
>> index 8712ee0d6..d0b765b60 100644
>> --- a/src/lua/fiber.lua
>> +++ b/src/lua/fiber.lua
>> @@ -34,4 +34,64 @@ fiber.time = fiber_time
>>  fiber.time64 = fiber_time64
>>  fiber.clock = fiber_clock
>>  fiber.clock64 = fiber_clock64
>> +
>> +local sleep_infinite = fiber.sleep_infinite
>> +fiber.sleep_infinite = nil
>> +
>> +local worker_next_task = nil
>> +local worker_last_task = nil
>> +local worker_fiber = nil
>> +
>> +--
>> +-- Worker is a singleton fiber for not urgent delayed execution of
>> +-- functions. Main purpose - schedule execution of a function,
>> +-- which is going to yield, from a context, where a yield is not
>> +-- allowed. Such as an FFI object's GC callback.
>> +--
>> +local function worker_f()
>> +    local task
>> +    while true do
>> +        while true do
>> +            task = worker_next_task
>> +            if task then
>> +                break
>> +            end
>> +            sleep_infinite()
>> +        end
>> +        worker_next_task = task.next
>> +        task.f(task.arg)
>> +        fiber.sleep(0)
>> +    end
>> +end
>> +
>> +local function worker_safe_f()
>> +    pcall(worker_f)
>> +    -- This fiber is probably canceled and now is not able to
>> +    -- sleep, create a new one.
> 
> Minor: I guess it worth mentioning the fact that <worker_f> doesn't
> return. It would definitely make the comment clearer.

Done.

====================
diff --git a/src/lua/fiber.lua b/src/lua/fiber.lua
index fa53403d0..89c17f63d 100644
--- a/src/lua/fiber.lua
+++ b/src/lua/fiber.lua
@@ -66,8 +66,9 @@ end
 
 local function worker_safe_f()
     pcall(worker_f)
-    -- This fiber is probably canceled and now is not able to
-    -- sleep, create a new one.
+    -- Worker_f never returns. If the execution is here, this
+    -- fiber is probably canceled and now is not able to sleep.
+    -- Create a new one.
     worker_fiber = fiber.new(worker_safe_f)
 end
====================

>> +    worker_fiber = fiber.new(worker_safe_f)
>> +end
>> +
>> +worker_fiber = fiber.new(worker_safe_f)
>> +
>> +local function worker_schedule_task(f, arg)
>> +    local task = {f = f, arg = arg}
>> +    if not worker_next_task then
>> +        worker_next_task = task
>> +    else
> 
> Minor: It would be great to add an assert here, since nothing except the
> current implementation prevents us from indexing a nil value. Feel free
> to ignore.
The assertion will explode with the same error at an attempt
to index nil - no information will be lost, but the assert
would be executed even in Release. Lets better omit it.

>> +        worker_last_task.next = task
>> +    end
>> +    worker_last_task = task
>> +    worker_fiber:wakeup()
>> +end
>> +
>> diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
>> index 6df210d9c..f10782d1f 100644
>> --- a/test/app/fiber.test.lua
>> +++ b/test/app/fiber.test.lua
>> @@ -688,6 +688,47 @@ tbl.time > 0
>>  fiber.top_disable()
>>  fiber.top()
>>  
>> +--
>> +-- fiber._internal.schedule_task() - API for internal usage for
>> +-- delayed execution of a function.
>> +--
>> +glob_arg = {}
>> +count = 0
>> +function task_f(arg)                                                            \
>> +    count = count + 1                                                           \
>> +    table.insert(glob_arg, arg)                                                 \
>> +    arg = arg + 1                                                               \
>> +    if arg <= 3 then                                                            \
>> +        fiber._internal.schedule_task(task_f, arg)                              \
>> +    else                                                                        \
>> +        fiber.self():cancel()                                                   \
>> +        error('Worker is broken')                                               \
>> +    end                                                                         \
>> +end
>> +for i = 1, 3 do                                                                 \
>> +    local csw1 = fiber.info()[fiber.id()].csw                                   \
>> +    fiber._internal.schedule_task(task_f, i)                                    \
>> +    local csw2 = fiber.info()[fiber.id()].csw                                   \
>> +    assert(csw1 == csw2 and csw1 ~= nil)                                        \
> 
> Minor: IMHO it's better to check that csw1 is a 'number'. Feel free to
> ignore.

I don't mind.

====================
diff --git a/test/app/fiber.result b/test/app/fiber.result
index bd60e1483..0e1ca815c 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1588,7 +1588,7 @@ for i = 1, 3 do
     local csw1 = fiber.info()[fiber.id()].csw                                   \
     fiber._internal.schedule_task(task_f, i)                                    \
     local csw2 = fiber.info()[fiber.id()].csw                                   \
-    assert(csw1 == csw2 and csw1 ~= nil)                                        \
+    assert(csw1 == csw2 and type(csw1) == 'number')                             \
 end
 ---
 ...
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index f10782d1f..f01144f49 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -709,7 +709,7 @@ for i = 1, 3 do
     local csw1 = fiber.info()[fiber.id()].csw                                   \
     fiber._internal.schedule_task(task_f, i)                                    \
     local csw2 = fiber.info()[fiber.id()].csw                                   \
-    assert(csw1 == csw2 and csw1 ~= nil)                                        \
+    assert(csw1 == csw2 and type(csw1) == 'number')                             \
 end
 old_count = count
 test_run:wait_cond(function()                                                   \
====================

Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

Indeed. It is cheaper and simpler to use cdata here.

On 19/03/2020 23:53, Igor Munkin wrote:
> Vlad,
> 
> As you mentioned to me, the doc[1] specify 'userdata' (i.e. opaque type)
> as a return type for <fio.open>. Current implementation returns a table,
> so as we discussed there is nothing preventing you from dropping all
> hacks you used in the patch and implement file handle object via cdata
> or userdata.

See new patch below.

====================

    fio: close unused descriptors automatically
    
    Fio.open() returned a file descriptor, which was not closed
    automatically after all its links were nullified. In other words,
    GC didn't close the descriptor.
    
    This was not really useful, because after fio.open() an exception
    may appear, and user needed to workaround this to manually call
    fio_object:close(). Also this was not consistent with io.open().
    
    Now fio.open() object closes the descriptor automatically when
    GCed.
    
    Closes #4727
    
    @TarantoolBot document
    Title: fio descriptor is closed automatically by GC
    
    fio.open() returns a descriptor which can be closed manually by
    calling :close() method, or it will be closed automatically, when
    it has no references, and GC deletes it.
    
    :close() method existed always, auto GC was added just now.
    
    Keep in mind, that the number of file descriptors is limited, and
    they can end earlier than GC will be triggered to collect not
    used descriptors. It is always better to close them manually as
    soon as possible.

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 4692e1026..d3c257b88 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -5,11 +5,16 @@ local ffi = require('ffi')
 local buffer = require('buffer')
 local fiber = require('fiber')
 local errno = require('errno')
+local schedule_task = fiber._internal.schedule_task
 
 ffi.cdef[[
     int umask(int mask);
     char *dirname(char *path);
     int chdir(const char *path);
+
+    struct fio_handle {
+        int fh;
+    };
 ]]
 
 local const_char_ptr_t = ffi.typeof('const char *')
@@ -160,7 +165,22 @@ fio_methods.stat = function(self)
     return internal.fstat(self.fh)
 end
 
-local fio_mt = { __index = fio_methods }
+fio_methods.__serialize = function(self)
+    return {fh = self.fh}
+end
+
+local fio_mt = {
+    __index = fio_methods,
+    __gc = function(obj)
+        if obj.fh >= 0 then
+            -- FFI GC can't yield. Internal.close() yields.
+            -- Collect the garbage later, in a worker fiber.
+            schedule_task(internal.close, obj.fh)
+        end
+    end,
+}
+
+ffi.metatype('struct fio_handle', fio_mt)
 
 fio.open = function(path, flags, mode)
     local iflag = 0
@@ -202,10 +222,13 @@ fio.open = function(path, flags, mode)
     if err ~= nil then
         return nil, err
     end
-
-    fh = { fh = fh }
-    setmetatable(fh, fio_mt)
-    return fh
+    local ok, res = pcall(ffi.new, 'struct fio_handle', fh)
+    if not ok then
+        internal.close(fh)
+        -- This is OOM.
+        return error(res)
+    end
+    return res
 end
 
 fio.pathjoin = function(...)
diff --git a/test/app/gh-4727-fio-gc.result b/test/app/gh-4727-fio-gc.result
new file mode 100644
index 000000000..d793eded3
--- /dev/null
+++ b/test/app/gh-4727-fio-gc.result
@@ -0,0 +1,104 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+fio = require('fio')
+ | ---
+ | ...
+--
+-- gh-4727: fio handler GC.
+--
+flags = {'O_CREAT', 'O_RDWR'}
+ | ---
+ | ...
+mode = {'S_IRWXU'}
+ | ---
+ | ...
+filename = 'test4727.txt'
+ | ---
+ | ...
+fh1 = nil
+ | ---
+ | ...
+fh2 = nil
+ | ---
+ | ...
+-- Idea of the test is that according to the Open Group standard,
+-- open() always returns the smallest available descriptor. This
+-- means, that in 'open() + close() + open()' the second open()
+-- should return the same value as the first call, if no other
+-- threads/fibers managed to interfere. Because of the
+-- interference the sequence may need to be called multiple times
+-- to catch a couple of equal descriptors.
+test_run:wait_cond(function()                                       \
+    collectgarbage('collect')                                       \
+    local f = fio.open(filename, flags, mode)                       \
+    fh1 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+-- GC function of a fio object works in a separate fiber. Give it   \
+-- time to execute.                                                 \
+    fiber.yield()                                                   \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+    fiber.yield()                                                   \
+    return fh1 == fh2                                               \
+end) or {fh1, fh2}
+ | ---
+ | - true
+ | ...
+
+-- Ensure, that GC does not break anything after explicit close.
+-- Idea of the test is the same as in the previous test, but now
+-- the second descriptor is used for something. If GC of the first
+-- fio object is called even after close(), it would close the
+-- same descriptor, already used by the second fio object. And it
+-- won't be able to write anything. Or will write, but to a
+-- totally different descriptor created by some other
+-- fiber/thread. This is why read() is called on the same file
+-- afterwards.
+f = nil
+ | ---
+ | ...
+test_run:wait_cond(function()                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh1 = f.fh                                                      \
+    f:close()                                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    return fh1 == fh2                                               \
+end)
+ | ---
+ | - true
+ | ...
+collectgarbage('collect')
+ | ---
+ | - 0
+ | ...
+fiber.yield()
+ | ---
+ | ...
+f:write('test')
+ | ---
+ | - true
+ | ...
+f:close()
+ | ---
+ | - true
+ | ...
+f = fio.open(filename, flags, mode)
+ | ---
+ | ...
+f:read()
+ | ---
+ | - test
+ | ...
+f:close()
+ | ---
+ | - true
+ | ...
diff --git a/test/app/gh-4727-fio-gc.test.lua b/test/app/gh-4727-fio-gc.test.lua
new file mode 100644
index 000000000..d0ab585ca
--- /dev/null
+++ b/test/app/gh-4727-fio-gc.test.lua
@@ -0,0 +1,60 @@
+test_run = require('test_run').new()
+fiber = require('fiber')
+fio = require('fio')
+--
+-- gh-4727: fio handler GC.
+--
+flags = {'O_CREAT', 'O_RDWR'}
+mode = {'S_IRWXU'}
+filename = 'test4727.txt'
+fh1 = nil
+fh2 = nil
+-- Idea of the test is that according to the Open Group standard,
+-- open() always returns the smallest available descriptor. This
+-- means, that in 'open() + close() + open()' the second open()
+-- should return the same value as the first call, if no other
+-- threads/fibers managed to interfere. Because of the
+-- interference the sequence may need to be called multiple times
+-- to catch a couple of equal descriptors.
+test_run:wait_cond(function()                                       \
+    collectgarbage('collect')                                       \
+    local f = fio.open(filename, flags, mode)                       \
+    fh1 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+-- GC function of a fio object works in a separate fiber. Give it   \
+-- time to execute.                                                 \
+    fiber.yield()                                                   \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    f = nil                                                         \
+    collectgarbage('collect')                                       \
+    fiber.yield()                                                   \
+    return fh1 == fh2                                               \
+end) or {fh1, fh2}
+
+-- Ensure, that GC does not break anything after explicit close.
+-- Idea of the test is the same as in the previous test, but now
+-- the second descriptor is used for something. If GC of the first
+-- fio object is called even after close(), it would close the
+-- same descriptor, already used by the second fio object. And it
+-- won't be able to write anything. Or will write, but to a
+-- totally different descriptor created by some other
+-- fiber/thread. This is why read() is called on the same file
+-- afterwards.
+f = nil
+test_run:wait_cond(function()                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh1 = f.fh                                                      \
+    f:close()                                                       \
+    f = fio.open(filename, flags, mode)                             \
+    fh2 = f.fh                                                      \
+    return fh1 == fh2                                               \
+end)
+collectgarbage('collect')
+fiber.yield()
+f:write('test')
+f:close()
+f = fio.open(filename, flags, mode)
+f:read()
+f:close()

Re: [Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function

[Igor Munkin]

Vlad,

Thanks for the patch, LGTM.

On 20.03.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Igor Munkin]

Vlad,

Thanks, the patch is neat! LGTM except the one comment below.

On 20.03.20, Vladislav Shpilevoy wrote:
> Indeed. It is cheaper and simpler to use cdata here.
> 
> On 19/03/2020 23:53, Igor Munkin wrote:
> > Vlad,
> > 
> > As you mentioned to me, the doc[1] specify 'userdata' (i.e. opaque type)
> > as a return type for <fio.open>. Current implementation returns a table,
> > so as we discussed there is nothing preventing you from dropping all
> > hacks you used in the patch and implement file handle object via cdata
> > or userdata.
> 
> See new patch below.
> 
> ====================
> 
>     fio: close unused descriptors automatically
>     
>     Fio.open() returned a file descriptor, which was not closed
>     automatically after all its links were nullified. In other words,
>     GC didn't close the descriptor.
>     
>     This was not really useful, because after fio.open() an exception
>     may appear, and user needed to workaround this to manually call
>     fio_object:close(). Also this was not consistent with io.open().
>     
>     Now fio.open() object closes the descriptor automatically when
>     GCed.
>     
>     Closes #4727
>     
>     @TarantoolBot document
>     Title: fio descriptor is closed automatically by GC
>     
>     fio.open() returns a descriptor which can be closed manually by
>     calling :close() method, or it will be closed automatically, when
>     it has no references, and GC deletes it.
>     
>     :close() method existed always, auto GC was added just now.
>     
>     Keep in mind, that the number of file descriptors is limited, and
>     they can end earlier than GC will be triggered to collect not
>     used descriptors. It is always better to close them manually as
>     soon as possible.
> 
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 4692e1026..d3c257b88 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua

<snipped>

> @@ -160,7 +165,22 @@ fio_methods.stat = function(self)
>      return internal.fstat(self.fh)
>  end
>  
> -local fio_mt = { __index = fio_methods }
> +fio_methods.__serialize = function(self)
> +    return {fh = self.fh}
> +end
> +
> +local fio_mt = {
> +    __index = fio_methods,
> +    __gc = function(obj)
> +        if obj.fh >= 0 then

Considering the current semantics of fio_methods.close (I added the code
below) if internal.close fails, retry is not triggered in __gc mm due to
the condition above. I guess you just missed to fix it here (you hoisted
the error handling in the previous patch).

================================================================================

fio_methods.close = function(self)
    local res, err = internal.close(self.fh)
    self.fh = -1
    if err ~= nil then
        return false, err
    end
    return res
end

================================================================================

> +            -- FFI GC can't yield. Internal.close() yields.
> +            -- Collect the garbage later, in a worker fiber.
> +            schedule_task(internal.close, obj.fh)
> +        end
> +    end,
> +}
> +
> +ffi.metatype('struct fio_handle', fio_mt)
>  
>  fio.open = function(path, flags, mode)
>      local iflag = 0
> @@ -202,10 +222,13 @@ fio.open = function(path, flags, mode)
>      if err ~= nil then
>          return nil, err
>      end
> -
> -    fh = { fh = fh }
> -    setmetatable(fh, fio_mt)
> -    return fh
> +    local ok, res = pcall(ffi.new, 'struct fio_handle', fh)
> +    if not ok then
> +        internal.close(fh)
> +        -- This is OOM.
> +        return error(res)
> +    end
> +    return res
>  end
>  
>  fio.pathjoin = function(...)

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

>> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
>> index 4692e1026..d3c257b88 100644
>> --- a/src/lua/fio.lua
>> +++ b/src/lua/fio.lua
> 
> <snipped>
> 
>> @@ -160,7 +165,22 @@ fio_methods.stat = function(self)
>>      return internal.fstat(self.fh)
>>  end
>>  
>> -local fio_mt = { __index = fio_methods }
>> +fio_methods.__serialize = function(self)
>> +    return {fh = self.fh}
>> +end
>> +
>> +local fio_mt = {
>> +    __index = fio_methods,
>> +    __gc = function(obj)
>> +        if obj.fh >= 0 then
> 
> Considering the current semantics of fio_methods.close (I added the code
> below) if internal.close fails, retry is not triggered in __gc mm due to
> the condition above. I guess you just missed to fix it here (you hoisted
> the error handling in the previous patch).

Yeah, I knew :) I just didn't want to touch this place.
But ok, here is a separate commit to fix this thing, since it
existed before my patch, and is an independent bug.

====================

commit 1dcd0e6ffb1916d38b374f6df634dfcf3c0a40bc
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Fri Mar 20 22:20:20 2020 +0100

    fio: on close() don't loose fd if internal close fails
    
    File descriptor was set to -1 regardless of whether
    the object was closed properly. As a result, in case
    of an error the descriptor would leak.
    
    GC finalizer of a descriptor is left intact not to
    overcomplicate it.

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index d3c257b88..83fddaa0a 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -146,10 +146,10 @@ end
 
 fio_methods.close = function(self)
     local res, err = internal.close(self.fh)
-    self.fh = -1
     if err ~= nil then
         return false, err
     end
+    self.fh = -1
     return res
 end
 

Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Igor Munkin]

Vlad,

Great, thanks! LGTM.

On 20.03.20, Vladislav Shpilevoy wrote:
> >> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> >> index 4692e1026..d3c257b88 100644
> >> --- a/src/lua/fio.lua
> >> +++ b/src/lua/fio.lua
> > 
> > <snipped>
> > 
> >> @@ -160,7 +165,22 @@ fio_methods.stat = function(self)
> >>      return internal.fstat(self.fh)
> >>  end
> >>  
> >> -local fio_mt = { __index = fio_methods }
> >> +fio_methods.__serialize = function(self)
> >> +    return {fh = self.fh}
> >> +end
> >> +
> >> +local fio_mt = {
> >> +    __index = fio_methods,
> >> +    __gc = function(obj)
> >> +        if obj.fh >= 0 then
> > 
> > Considering the current semantics of fio_methods.close (I added the code
> > below) if internal.close fails, retry is not triggered in __gc mm due to
> > the condition above. I guess you just missed to fix it here (you hoisted
> > the error handling in the previous patch).
> 
> Yeah, I knew :) I just didn't want to touch this place.
> But ok, here is a separate commit to fix this thing, since it
> existed before my patch, and is an independent bug.
> 
> ====================
> 
> commit 1dcd0e6ffb1916d38b374f6df634dfcf3c0a40bc
> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Date:   Fri Mar 20 22:20:20 2020 +0100
> 
>     fio: on close() don't loose fd if internal close fails
>     
>     File descriptor was set to -1 regardless of whether
>     the object was closed properly. As a result, in case
>     of an error the descriptor would leak.
>     
>     GC finalizer of a descriptor is left intact not to
>     overcomplicate it.
> 
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index d3c257b88..83fddaa0a 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -146,10 +146,10 @@ end
>  
>  fio_methods.close = function(self)
>      local res, err = internal.close(self.fh)
> -    self.fh = -1
>      if err ~= nil then
>          return false, err
>      end
> +    self.fh = -1
>      return res
>  end
>  

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 0/3] fio: close unused descriptors automatically

[Nikita Pettik]

On 03 Mar 00:29, Vladislav Shpilevoy wrote:
> The patchset introduces a fiber worker singleton object and uses
> it for garbage collection of fio and SWIM objects. This is a
> workaround for inability to yield in ffi.gc hook, which is used by
> fio and SWIM.
> 
> SWIM had GC before, but it was creating a new fiber for each GC.
> Fio didn't have GC.

LGMT (I looked at fresh version on your branch).
 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4727-fio-gc
> Issue: https://github.com/tarantool/tarantool/issues/4727
> 
> @ChangeLog
> - fio descriptors are closed on garbage collection (gh-4727).
> 
> Changes in V2:
> - Added a special single fiber for garbage collection of objects,
>   which yield in their destructors.
> 
> Vladislav Shpilevoy (3):
>   fiber: introduce schedule_task() internal function
>   fio: close unused descriptors automatically
>   swim: use fiber._internal.schedule_task() for GC
> 
>  src/lua/fiber.c                  |  15 +++++
>  src/lua/fiber.lua                |  60 ++++++++++++++++++
>  src/lua/fio.lua                  |  34 ++++++++--
>  src/lua/swim.lua                 |  11 ++--
>  test/app/fiber.result            |  72 +++++++++++++++++++++
>  test/app/fiber.test.lua          |  41 ++++++++++++
>  test/app/gh-4727-fio-gc.result   | 104 +++++++++++++++++++++++++++++++
>  test/app/gh-4727-fio-gc.test.lua |  60 ++++++++++++++++++
>  8 files changed, 385 insertions(+), 12 deletions(-)
>  create mode 100644 test/app/gh-4727-fio-gc.result
>  create mode 100644 test/app/gh-4727-fio-gc.test.lua
> 
> -- 
> 2.21.1 (Apple Git-122.3)
> 

Re: [Tarantool-patches] [PATCH v2 0/3] fio: close unused descriptors automatically

[Kirill Yukhin]

Hello,

On 03 мар 00:29, Vladislav Shpilevoy wrote:
> The patchset introduces a fiber worker singleton object and uses
> it for garbage collection of fio and SWIM objects. This is a
> workaround for inability to yield in ffi.gc hook, which is used by
> fio and SWIM.
> 
> SWIM had GC before, but it was creating a new fiber for each GC.
> Fio didn't have GC.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4727-fio-gc
> Issue: https://github.com/tarantool/tarantool/issues/4727
> 
> @ChangeLog
> - fio descriptors are closed on garbage collection (gh-4727).

I've checked your patchset into master.

--
Regards, Kirill Yukhin