[Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically
Konstantin Osipov
kostja.osipov at gmail.com
Sun Feb 9 22:25:24 MSK 2020
* Vladislav Shpilevoy <v.shpilevoy at 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
More information about the Tarantool-patches
mailing list