From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f194.google.com (mail-lj1-f194.google.com [209.85.208.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8F66A46970E for ; Sun, 9 Feb 2020 22:25:28 +0300 (MSK) Received: by mail-lj1-f194.google.com with SMTP id r19so4683208ljg.3 for ; Sun, 09 Feb 2020 11:25:28 -0800 (PST) Date: Sun, 9 Feb 2020 22:25:24 +0300 From: Konstantin Osipov Message-ID: <20200209192524.GA16448@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [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