From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 CDD4046970E for ; Sun, 9 Feb 2020 21:34:39 +0300 (MSK) From: Georgy Kirichenko Date: Sun, 09 Feb 2020 21:34:38 +0300 Message-ID: <4600369.31r3eYUQgx@localhost> In-Reply-To: References: MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4275015.LvFx2qVVIh"; micalg="pgp-sha256"; protocol="application/pgp-signature" 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: tarantool-patches@dev.tarantool.org, korablev@tarantool.org, imun@tarantool.org Cc: Vladislav Shpilevoy --nextPart4275015.LvFx2qVVIh Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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() --nextPart4275015.LvFx2qVVIh Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEECXG+Yw5ArYcP8x5wDVHWG5PoUw4FAl5AUL4ACgkQDVHWG5Po Uw6qKQf/bX7bEHrbvOutkkf5BAgwoum6C7YSJwsicI+jwTBd9q34A6vwrJ1hHYGw fCTmZHlCO+jepc71wSgZDlWa/i7Wns0/q5WlpcBVVQSG/rCf/dnW7jxIBgYwtDVa n/DjAAdVuvJmtkDXBic7LCF3b+G/L7SOziw6Cw0nSCxoPudEcjf+A7o+PpQEVbA2 sqXZTmCXCB7DAz/9qa476lDIwIdqFLNwn6vHrUXkI6uOPhbiPqCOzcjQ41Jd5aDs mmrwjsz7GqliYTs+SAE/rXgvVi/zTCX/3LVbvePQelJc3fMIJVgWN6M8C2R4dhNp UNWHUR6poMNsvjsEo2vzMeJ96kn6HA== =78uY -----END PGP SIGNATURE----- --nextPart4275015.LvFx2qVVIh--