[Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

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()
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

Sorry, forgot the branch and issue:

Issue: https://github.com/tarantool/tarantool/issues/4727
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4727-fio-gc

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Georgy Kirichenko]

[-- Attachment #1: Type: text/plain, Size: 9792 bytes --]

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


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

Hi! Thanks for your review!

On 09/02/2020 19:34, Georgy Kirichenko wrote:
> 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.

Good catch!

================================================================================
             -- FFI GC can't yield. Internal.close() yields.
             -- Collect the garbage later, in a separate fiber.
-            fiber.create(internal.close, fh)
+            fiber.new(internal.close, fh)
         end)
     }, fio_mt)
================================================================================

> From my point of view the possible approach is a dedicated fiber with
> a list of pending callbacks.
Yes, I was thinking about this as well, but thought it would be an overkill
just for fio. Since you also propose this solution, I suggest to do it
separately, and for other modules too. We could introduce a new module
'background', or 'gc', or 'worker', or 'pool' which would provide API like
'execute(func, arg)'. And it would call it in a special global fiber.

Example:

    pool = require('pool')
    pool.execute(internal.close, fd)

So basically we could reinvent fiber pool for Lua.

This can be used for fio and SWIM objects GC, because SWIM currently
also creates a new fiber in its GC callback.

Also this could be implemented as a submodule of fiber module. For instance,
fiber.pool.execute().

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

It was decided, that Georgy is right. I will create a new
commit on this branch, which introduces a special fiber for
delayed GC. I will use it in SWIM and in fio.

So please, skip this version. A new one will be send as v2.

On 09/02/2020 19:34, Georgy Kirichenko wrote:
> 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()
> 

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/02/09 21:55]:

While I agree with the patch ,the reason it wasn't done was most
likely performance/reducing the number of finalizers.

> 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()
> -- 
> 2.21.1 (Apple Git-122.3)

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Kirill Yukhin]

Hello,

On 09 фев 18:37, Vladislav Shpilevoy wrote:
>  test/app/fio.result   | 93 +++++++++++++++++++++++++++++++++++++++++++
>  test/app/fio.test.lua | 58 +++++++++++++++++++++++++++

Could you please put the test into separate file?

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Cyrill Gorcunov]

On Mon, Feb 10, 2020 at 09:31:44AM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 09 фев 18:37, Vladislav Shpilevoy wrote:
> >  test/app/fio.result   | 93 +++++++++++++++++++++++++++++++++++++++++++
> >  test/app/fio.test.lua | 58 +++++++++++++++++++++++++++
> 
> Could you please put the test into separate file?

Sure, will do.

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Cyrill Gorcunov]

On Mon, Feb 10, 2020 at 11:20:06AM +0300, Cyrill Gorcunov wrote:
> > 
> > Could you please put the test into separate file?
> 
> Sure, will do.

Oh, this mail wasn't addressed to me :-) Sorry.

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Vladislav Shpilevoy]

Hi!

On 10/02/2020 07:31, Kirill Yukhin wrote:
> Hello,
> 
> On 09 фев 18:37, Vladislav Shpilevoy wrote:
>>  test/app/fio.result   | 93 +++++++++++++++++++++++++++++++++++++++++++
>>  test/app/fio.test.lua | 58 +++++++++++++++++++++++++++
> 
> Could you please put the test into separate file?

It contradicts with SOP.

    "
     We do not create a separate file for each feature - that
     would lead to a huge number of files. For a bug fix,
     please create a separate file with autonomous test case in
     a correponding test suite. Name should follow this format:
     <suite name>/gh-XXX-two-three-word-title
    "

This is not a bugfix. It is a feature. Auto GC of FIO was never
documented anywhere, so this was not a bug. And according to SOP
it should not be in a separate file. But ok, I moved it.

> --
> Regards, Kirill Yukhin
> 

Re: [Tarantool-patches] [PATCH 1/1] fio: close unused descriptors automatically

[Kirill Yukhin]

Hello,

On 10 фев 20:36, Vladislav Shpilevoy wrote:
> Hi!
> 
> On 10/02/2020 07:31, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 09 фев 18:37, Vladislav Shpilevoy wrote:
> >>  test/app/fio.result   | 93 +++++++++++++++++++++++++++++++++++++++++++
> >>  test/app/fio.test.lua | 58 +++++++++++++++++++++++++++
> > 
> > Could you please put the test into separate file?
> 
> It contradicts with SOP.
> 
>     "
>      We do not create a separate file for each feature - that
>      would lead to a huge number of files. For a bug fix,
>      please create a separate file with autonomous test case in
>      a correponding test suite. Name should follow this format:
>      <suite name>/gh-XXX-two-three-word-title
>     "
> 
> This is not a bugfix. It is a feature. Auto GC of FIO was never
> documented anywhere, so this was not a bug. And according to SOP
> it should not be in a separate file. But ok, I moved it.

Thanks for doing this and for mentioning that this is actually a
feature. I've chanched corresponding label of the issue and
moved it to latest MS since we do not commit features to release
branches.

--
Regards, Kirill Yukhin