[Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Mar 2 23:51:10 MSK 2020
It was decided, that Georgy is right. I will create a new
commit on this branch, which introduces a special fiber for
delayed GC. I will use it in SWIM and in fio.
So please, skip this version. A new one will be send as v2.
On 09/02/2020 19:34, Georgy Kirichenko wrote:
> Hi, thanks for the patch.
>
> Unfortunately your patch is not correct because fiber.create
> yields (which could be fixed by using fiber.new) and raises an error what is not
> allowed inside gc callbacks.
>
> From my point of view the possible approach is a dedicated fiber with
> a list of pending callbacks.
>
> WBR
>
> On Sunday, 9 February 2020 20:37:09 MSK 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.
>>
>> ---
>>
>> @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()
>
More information about the Tarantool-patches
mailing list