Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically

[Igor Munkin <imun@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 <fio.open>. 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

<snipped>

> @@ -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(...)

<snipped>

-- 
Best regards,
IM