From: Igor Munkin <imun@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically
Date: Sat, 21 Mar 2020 00:28:54 +0300 [thread overview]
Message-ID: <20200320212854.GB22874@tarantool.org> (raw)
In-Reply-To: <19736aa3-aa20-e56a-8ea5-4a40f6c4c8e4@tarantool.org>
Vlad,
Great, thanks! LGTM.
On 20.03.20, Vladislav Shpilevoy wrote:
> >> 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).
>
> Yeah, I knew :) I just didn't want to touch this place.
> But ok, here is a separate commit to fix this thing, since it
> existed before my patch, and is an independent bug.
>
> ====================
>
> commit 1dcd0e6ffb1916d38b374f6df634dfcf3c0a40bc
> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Date: Fri Mar 20 22:20:20 2020 +0100
>
> fio: on close() don't loose fd if internal close fails
>
> File descriptor was set to -1 regardless of whether
> the object was closed properly. As a result, in case
> of an error the descriptor would leak.
>
> GC finalizer of a descriptor is left intact not to
> overcomplicate it.
>
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index d3c257b88..83fddaa0a 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -146,10 +146,10 @@ end
>
> fio_methods.close = function(self)
> local res, err = internal.close(self.fh)
> - self.fh = -1
> if err ~= nil then
> return false, err
> end
> + self.fh = -1
> return res
> end
>
--
Best regards,
IM
next prev parent reply other threads:[~2020-03-20 21:35 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 23:29 [Tarantool-patches] [PATCH v2 0/3] " Vladislav Shpilevoy
2020-03-02 23:29 ` [Tarantool-patches] [PATCH v2 1/3] fiber: introduce schedule_task() internal function Vladislav Shpilevoy
2020-03-19 14:52 ` Igor Munkin
2020-03-20 0:00 ` Vladislav Shpilevoy
2020-03-20 10:48 ` Igor Munkin
2020-03-02 23:29 ` [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically Vladislav Shpilevoy
2020-03-19 14:53 ` Igor Munkin
2020-03-19 22:53 ` Igor Munkin
2020-03-20 0:00 ` Vladislav Shpilevoy
2020-03-20 10:48 ` Igor Munkin
2020-03-20 21:28 ` Vladislav Shpilevoy
2020-03-20 21:28 ` Igor Munkin [this message]
2020-03-02 23:29 ` [Tarantool-patches] [PATCH v2 3/3] swim: use fiber._internal.schedule_task() for GC Vladislav Shpilevoy
2020-03-19 14:53 ` Igor Munkin
2020-03-26 1:08 ` [Tarantool-patches] [PATCH v2 0/3] fio: close unused descriptors automatically Nikita Pettik
2020-03-26 12:56 ` Kirill Yukhin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200320212854.GB22874@tarantool.org \
--to=imun@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--cc=v.shpilevoy@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v2 2/3] fio: close unused descriptors automatically' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox