[Tarantool-patches] [PATCH v2] fio: fix race condition in mktree

[Tarantool-patches] [PATCH v2] fio: fix race condition in mktree

[HustonMmmavr]

Despite the lack of documentation, fio.mktree() was designed to work
similar to mkdir -p: it creates the directory along with it's parents
and doesn't complain about existing ones.

But this function was subject to a race if two different processes were
trying to create the same directory at the same time. It was caused by
the fact that directory existence check and its creation aren't atomic.

This patch fixes the race by impoving error handling: it's not an error
if directory exists, even if it was created by someone else and mktree
failed.

Related to https://github.com/tarantool/doc/issues/1063
Closes #4660
---
branch: https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-4660-fix-fio-mktree-race

 src/lua/fio.lua | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index cb224f3d0..c9aeac951 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -364,7 +364,7 @@ fio.mktree = function(path, mode)
         local stat = fio.stat(current_dir)
         if stat == nil then
             local st, err = fio.mkdir(current_dir, mode)
-            if err ~= nil  then
+            if err ~= nil and not fio.path.is_dir(current_dir) then
                 return false, string.format("Error creating directory %s: %s",
                     current_dir, tostring(err))
             end
-- 
2.23.0

Re: [Tarantool-patches] [PATCH v2] fio: fix race condition in mktree

[Igor Munkin]

Hello,

Thanks, LGTM.

Cced Sasha Tu. to proceed with the patch.

On 13.01.20, HustonMmmavr wrote:
> Despite the lack of documentation, fio.mktree() was designed to work
> similar to mkdir -p: it creates the directory along with it's parents
> and doesn't complain about existing ones.
> 
> But this function was subject to a race if two different processes were
> trying to create the same directory at the same time. It was caused by
> the fact that directory existence check and its creation aren't atomic.
> 
> This patch fixes the race by impoving error handling: it's not an error
> if directory exists, even if it was created by someone else and mktree
> failed.
> 
> Related to https://github.com/tarantool/doc/issues/1063

Side note: you can mention the ticket from other repo queue the
following way:
| Relates to tarantool/doc#1063

> Closes #4660
> ---
> branch: https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-4660-fix-fio-mktree-race
> 
>  src/lua/fio.lua | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index cb224f3d0..c9aeac951 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -364,7 +364,7 @@ fio.mktree = function(path, mode)
>          local stat = fio.stat(current_dir)
>          if stat == nil then
>              local st, err = fio.mkdir(current_dir, mode)
> -            if err ~= nil  then
> +            if err ~= nil and not fio.path.is_dir(current_dir) then
>                  return false, string.format("Error creating directory %s: %s",
>                      current_dir, tostring(err))
>              end
> -- 
> 2.23.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2] fio: fix race condition in mktree

[Alexander Turenko]

Added the comment to the code:

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index c9aeac951..4692e1026 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -364,6 +364,10 @@ fio.mktree = function(path, mode)
         local stat = fio.stat(current_dir)
         if stat == nil then
             local st, err = fio.mkdir(current_dir, mode)
+            -- fio.stat() and fio.mkdir() above are separate calls
+            -- and a file system may be changed between them. So
+            -- if the error here is due to an existing directory,
+            -- the function should not report an error.
             if err ~= nil and not fio.path.is_dir(current_dir) then
                 return false, string.format("Error creating directory %s: %s",
                     current_dir, tostring(err))

Pushed to master, 2.3, 2.2 and 1.10.

> > Related to https://github.com/tarantool/doc/issues/1063
> 
> Side note: you can mention the ticket from other repo queue the
> following way:
> | Relates to tarantool/doc#1063

Don't sure I would prefer this way: a link is simpler for me. I left it
as an https link.