From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 01AC7469719 for ; Mon, 2 Mar 2020 23:51:12 +0300 (MSK) References: <4600369.31r3eYUQgx@localhost> From: Vladislav Shpilevoy Message-ID: <66f6cdbb-03b8-14ac-5b53-58d9919740ee@tarantool.org> Date: Mon, 2 Mar 2020 21:51:10 +0100 MIME-Version: 1.0 In-Reply-To: <4600369.31r3eYUQgx@localhost> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Georgy Kirichenko , tarantool-patches@dev.tarantool.org, korablev@tarantool.org, imun@tarantool.org 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() >