From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 36637469719 for ; Fri, 20 Mar 2020 13:54:41 +0300 (MSK) Date: Fri, 20 Mar 2020 13:48:23 +0300 From: Igor Munkin Message-ID: <20200320104823.GQ6392@tarantool.org> References: <20200319145310.GK6392@tarantool.org> <20200319225339.GO6392@tarantool.org> 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, the patch is neat! LGTM except the one comment below. On 20.03.20, Vladislav Shpilevoy wrote: > Indeed. It is cheaper and simpler to use cdata here. > > On 19/03/2020 23:53, Igor Munkin wrote: > > Vlad, > > > > As you mentioned to me, the doc[1] specify 'userdata' (i.e. opaque type) > > as a return type for . Current implementation returns a table, > > so as we discussed there is nothing preventing you from dropping all > > hacks you used in the patch and implement file handle object via cdata > > or userdata. > > See new patch below. > > ==================== > > fio: close unused descriptors automatically > > 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. > > Keep in mind, that the number of file descriptors is limited, and > they can end earlier than GC will be triggered to collect not > used descriptors. It is always better to close them manually as > soon as possible. > > diff --git a/src/lua/fio.lua b/src/lua/fio.lua > index 4692e1026..d3c257b88 100644 > --- a/src/lua/fio.lua > +++ b/src/lua/fio.lua > @@ -160,7 +165,22 @@ fio_methods.stat = function(self) > return internal.fstat(self.fh) > end > > -local fio_mt = { __index = fio_methods } > +fio_methods.__serialize = function(self) > + return {fh = self.fh} > +end > + > +local fio_mt = { > + __index = fio_methods, > + __gc = function(obj) > + if obj.fh >= 0 then Considering the current semantics of fio_methods.close (I added the code below) if internal.close fails, retry is not triggered in __gc mm due to the condition above. I guess you just missed to fix it here (you hoisted the error handling in the previous patch). ================================================================================ fio_methods.close = function(self) local res, err = internal.close(self.fh) self.fh = -1 if err ~= nil then return false, err end return res end ================================================================================ > + -- FFI GC can't yield. Internal.close() yields. > + -- Collect the garbage later, in a worker fiber. > + schedule_task(internal.close, obj.fh) > + end > + end, > +} > + > +ffi.metatype('struct fio_handle', fio_mt) > > fio.open = function(path, flags, mode) > local iflag = 0 > @@ -202,10 +222,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(ffi.new, 'struct fio_handle', fh) > + if not ok then > + internal.close(fh) > + -- This is OOM. > + return error(res) > + end > + return res > end > > fio.pathjoin = function(...) -- Best regards, IM