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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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)