From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 D074B469719 for ; Thu, 19 Mar 2020 17:59:27 +0300 (MSK) Date: Thu, 19 Mar 2020 17:53:10 +0300 From: Igor Munkin Message-ID: <20200319145310.GK6392@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 2/3] 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 Vlad, Thanks for the patch! I see no flaws in it, except the _gc field presence in fio object. I made another solution based on your cool hacks with ffi.gc (you can find the diff below). Feel free to disregard my changes if the problem doesn't bother you. On 03.03.20, 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. > --- > src/lua/fio.lua | 34 ++++++++-- > test/app/gh-4727-fio-gc.result | 104 +++++++++++++++++++++++++++++++ > test/app/gh-4727-fio-gc.test.lua | 60 ++++++++++++++++++ > 3 files changed, 192 insertions(+), 6 deletions(-) > create mode 100644 test/app/gh-4727-fio-gc.result > create mode 100644 test/app/gh-4727-fio-gc.test.lua > > diff --git a/src/lua/fio.lua b/src/lua/fio.lua > index 4692e1026..1ffb2ebfd 100644 > --- a/src/lua/fio.lua > +++ b/src/lua/fio.lua > @@ -5,6 +5,7 @@ local ffi = require('ffi') > local buffer = require('buffer') > local fiber = require('fiber') > local errno = require('errno') > +local schedule_task = fiber._internal.schedule_task > > ffi.cdef[[ > int umask(int mask); > @@ -141,10 +142,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 +163,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 worker fiber. > + schedule_task(internal.close, fh) > + end) > + }, fio_mt) > +end > > fio.open = function(path, flags, mode) > local iflag = 0 > @@ -202,10 +221,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(...) > -- > 2.21.1 (Apple Git-122.3) > ================================================================================ diff --git a/src/lua/fio.lua b/src/lua/fio.lua index 1ffb2ebfd..595ee175d 100644 --- a/src/lua/fio.lua +++ b/src/lua/fio.lua @@ -145,8 +145,7 @@ fio_methods.close = function(self) if err ~= nil then return false, err end - ffi.gc(self._gc, nil) - self._gc = nil + ffi.gc(getmetatable(self).__close, nil) self.fh = -1 return res end @@ -163,22 +162,17 @@ fio_methods.stat = function(self) return internal.fstat(self.fh) end -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() + }, { + __index = fio_methods, + __close = ffi.gc(ffi.new('char[1]'), function() -- FFI GC can't yield. Internal.close() yields. -- Collect the garbage later, in a worker fiber. schedule_task(internal.close, fh) end) - }, fio_mt) + }) end fio.open = function(path, flags, mode) ================================================================================ -- Best regards, IM