Race condition in std::fs::create_dir_all.

[dpc]

I just got bitten by this, took me a long time to find it and I see the issue was raised before in #30152

It's very non-obvious that std::fs::create_dir_all - which whole purpose of is to make sure the dir exists, can fail due to the fact that the dir was created concurrently.

The PR to fix it #30152 was rejected due to "Concurrent operations can't always be detected and when they do happen on the filesystem it often indicates that something else is going awry and needs to be kicked up further."

But please note that std::fs::create_dir_all is a convenience function, and not one directly mapping to a posix fs operation. If stdlib provides a convenience function, I think it should provide the sanest, and most robust one - not one with a big gotcha inside. The fact that there's a race condition between multiple calls to std::fs::create_dir_all is entirely a implementation choice and just a wrong behavior.

IMO any sane implementation should consider not takign care of this race condition a bug:

• http://opensource.apple.com/source/file_cmds/file_cmds-212/mkdir/mkdir.c
• http://stackoverflow.com/questions/675039/how-can-i-create-directory-tree-in-c-linux#comment15173179_675193

I've checked some other standard libraries and all are doing the right thing:

• Python: https://github.com/python/cpython/blob/c6ae2fcc724cbebb14e7c434b89eabdd64802cb3/Lib/os.py#L233
• Java: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/io/File.java#1209
• Boost: http://www.boost.org/doc/libs/1_39_0/boost/filesystem/convenience.hpp - argument that boost had this race condition, was used to close Fix race condition in fs::create_dir_all #30152 but it's not true - please see:

    BOOST_FS_FUNC(bool) create_directories(const Path& ph)
    {
         if (ph.empty() || exists(ph))
         {
           if ( !ph.empty() && !is_directory(ph) )
               boost::throw_exception( basic_filesystem_error<Path>(
                 "boost::filesystem::create_directories", ph,
                 make_error_code( boost::system::posix::file_exists ) ) );
           return false;
         }

         // First create branch, by calling ourself recursively
         create_directories(ph.parent_path());
         // Now that parent's path exists, create the directory
         create_directory(ph);
         return true;
     }

Note that return value of create_directories(ph.parent_path()); is ignored, not returned upwards, like in Rust implementation. And exception is thrown only if ph exists but is not a directory.

Think how many other Rust projects might have undetected race condition as they assumed the create_dir_all will do the right thing. Short googling already pointed other project that got bitten: https://github.com/droundy/bigbro/blob/464bfae81de4ad695a158315e3c75d6d21e382a2/src/lib.rs#L43

[alexcrichton]

The libs team discussed this during triage yesterday and the conclusion was that this does indeed seem ok to implement. Given the new information about other implementations along with a shift in intention for the "nice APIs" we provide like this and remove_dir_all, it also seems appropriate.

@dpc do you wanna send a PR?

[alexcrichton]

Ah it was also brought up that we probably want to alter the documentation as well to indicate what version of Rust includes the new behavior.

[dpc]

I don't really have a Rust development setup nor the experience with it, so I'll not be able to provide a PR. But isn't #30152 still applicable?

[alexcrichton]

Yeah it should still work, we'd probably just want to add some more tests as well

[rocallahan]

Can't this be closed now that #39799 is merged?