Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/02/09 21:55]:

While I agree with the patch ,the reason it wasn't done was most
likely performance/reducing the number of finalizers.

> 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.
> ---
> 
> @ChangeLog
> - fio descriptors are closed on garbage collection (gh-4727).
> 
>  src/lua/fio.lua       | 33 ++++++++++++---
>  test/app/fio.result   | 93 +++++++++++++++++++++++++++++++++++++++++++
>  test/app/fio.test.lua | 58 +++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+), 6 deletions(-)
> 
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 4692e1026..41901904c 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -141,10 +141,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 +162,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 separate fiber.
> +            fiber.create(internal.close, fh)
> +        end)
> +    }, fio_mt)
> +end
>  
>  fio.open = function(path, flags, mode)
>      local iflag = 0
> @@ -202,10 +220,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/fio.result b/test/app/fio.result
> index f83c43f44..6345ac22e 100644
> --- a/test/app/fio.result
> +++ b/test/app/fio.result
> @@ -1456,3 +1456,96 @@ fio.mktree('/dev/null/dir')
>  - false
>  - 'Error creating directory /dev/null: File exists'
>  ...
> +--
> +-- 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.
> +-- GC function of a fio object creates a new fiber. Give it         \
> +-- time to execute.                                                 \
> +test_run:wait_cond(function()                                       \
> +    collectgarbage('collect')                                       \
> +    local f = fio.open(filename, flags, mode)                       \
> +    fh1 = f.fh                                                      \
> +    f = nil                                                         \
> +    collectgarbage('collect')                                       \
> +    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/fio.test.lua b/test/app/fio.test.lua
> index 56c957d8a..c726bade6 100644
> --- a/test/app/fio.test.lua
> +++ b/test/app/fio.test.lua
> @@ -474,3 +474,61 @@ test_run:cmd("clear filter")
>  --
>  fio.mktree('/dev/null')
>  fio.mktree('/dev/null/dir')
> +
> +--
> +-- 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 creates a new 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)

-- 
Konstantin Osipov, Moscow, Russia