createDirectoryIfMissing should be atomic

[EricKow]

If I run createDirectoryIfMissing many times in parallel, it sometimes fails because (presumably) somewhere between the check if the directory exists, and between the actual creation, something else creates the directory.

Right now my workaround is to catch exceptions with isAlreadyExistsErrors

[duncan]

Unfortunately it's not clear that this is possible. The POSIX mkdir() function can returns EEXIST however that does not distinguish the case that there is a existing directory, or that there is an existing file (or indeed symlink).

It could be atomic but the error code / exception could not necessarily be accurate. It could check afterwards if the path that already existed was a file or directory, but of course that will not be atomic.

[EricKow]

Replying to duncan:

It could be atomic but the error code / exception could not necessarily be accurate. It could check afterwards if the path that already existed was a file or directory, but of course that will not be atomic.

Hmm, ok, maybe I don't want atomicity but the illusion of it :-) Actually, I'm not sure if this is the right thing for me to want, but I might like some sort of check afterwards (so createDirectoryIfMissing should catch its createDirectory exception, and if the path really is a directory, ignore the exception)... basically whatever it takes to let me continue using this function in my blissful ignorance.

[simonmar]

I've fixed the obvious race condition, but as Duncan says it's not possible to make this function completely race-free. I think this is enough to keep Eric happy, though.

Wed Nov 26 11:56:06 GMT 2008  Simon Marlow <marlowsd@gmail.com>
  * add test for createDirectoryIfMissing (#2808)

Wed Nov 26 12:36:59 GMT 2008  Simon Marlow <marlowsd@gmail.com>
  * avoid race conditions in createDirectoryIfMissing (#2808)

For reference the code is now:

createDirectoryIfMissing create_parents "" = return ()
createDirectoryIfMissing create_parents path0
 = do  r <- try $ createDirectory path
       case (r :: Either IOException ()) of
          Right _ -> return ()
          Left e
             | isAlreadyExistsError e -> return ()
             | isDoesNotExistError  e && create_parents -> do
                 createDirectoryIfMissing True (dropFileName path)
                 createDirectoryIfMissing True path
             | otherwise -> throw e
  where
    -- we want createDirectoryIfMissing "a/" to behave like   
    -- createDirectoryIfMissing "a".  Also, unless we apply
    -- dropTrailingPathSeparator first, dropFileName won't drop
    -- anything from "a/".
    path = dropTrailingPathSeparator path0

[igloo]

Both merged.

[duncan]

There are a few question marks with the new code:

The code is now race-free but the error codes are not accurate. createDirectoryIfMissing True "foo" will succeed if a file foo exists.

It is not entirely clear that it guarantees not to recurse infinitely. The termination condition relies on dropFileName . dropTrailingPathSeparator making the path shorter, on some base directory existing or being created or on an exception other than isDoesNotExistError being generated by createDirectory.

It could create the same directory again and again if some other thread or process deleted the directories it had already created. There is the danger of it getting into a fight with another process doing rm -r in a dir where createDirectoryIfMissing is creating dirs.

A fix for the third issue would be to change the second createDirectoryIfMissing call to use False.

Here is an alternative implementation for review. I think it addresses the three points above.

import System.Directory (createDirectory, doesDirectoryExist)
import System.FilePath
import System.IO.Error hiding (try)
import Control.Exception

createDirectoryIfMissing :: Bool -> FilePath -> IO ()
createDirectoryIfMissing _              ""   = return ()
createDirectoryIfMissing create_parents dir0
  | create_parents = createDirs (parents dir0)
  | otherwise      = createDir dir0 throw
  where
    parents = reverse . scanl1 (++) . splitPath . normalise

    createDirs []         = return ()
    createDirs (dir:dirs) =
      createDir dir $ \_ -> do
        createDirs dirs
        createDir dir throw
    
    createDir :: FilePath -> (IOException -> IO ()) -> IO ()
    createDir dir notExistHandler = do
      r <- try $ createDirectory dir
      case (r :: Either IOException ()) of
        Right ()                   -> return ()
        Left  e
          | isDoesNotExistError  e -> notExistHandler e
          -- createDirectory (and indeed POSIX mkdir) does not distinguish
          -- between a dir already existing and a file already existing. So we
          -- check for it here. Unfortunately there is a slight race condition
          -- here, but we think it is benign. It could report an exeption in
          -- the case that the dir did exist but another process deletes it
          -- before we can check that it did indeed exist.
          | isAlreadyExistsError e -> do exists <- doesDirectoryExist dir
                                         if exists then return ()
                                                   else throw e
          | otherwise              -> throw e

[duncan]

Updated to address Simon's comments and to handle the two create_parents cases in a more similar way, especially when it comes to trailing directory separators.

Ignoring comments it looks like:

createDirectoryIfMissing create_parents path0
  | create_parents = createDirs (parents path0)
  | otherwise      = createDirs (take 1 (parents path0))
  where
    parents = reverse . scanl1 (</>) . splitDirectories . normalise

    createDirs []         = return ()
    createDirs (dir:[])   = createDir dir throw
    createDirs (dir:dirs) =
      createDir dir $ \_ -> do
        createDirs dirs
        createDir dir throw

    createDir :: FilePath -> (IOException -> IO ()) -> IO ()
    createDir dir notExistHandler = do
      r <- try $ createDirectory dir
      case (r :: Either IOException ()) of
        Right ()                   -> return ()
        Left  e
          | isDoesNotExistError  e -> notExistHandler e
          | isAlreadyExistsError e -> do
              exists <- doesDirectoryExist dir
              if exists then return ()
                        else throw e
          | otherwise              -> throw e

There are two changes, one is to use scanl1 (</>) . splitDirectories instead of scanl1 (++) . splitPath. The other is to add a clause:

    createDirs (dir:[])   = createDir dir throw

So that we do not attempt to create the last dir twice. When it fails there's not more parent dirs to try so it's a hard failure.

That also means we can use createDirs in the not create_parents case. We don't just pass head (parents path0) because it will be empty when path0 == "". Using take 1 covers both cases.

[simonmar]

Dropping some notes here so I don't forget over the holidays:

• This new version fails createDirectory001, because the dir gets removed between createDirectory and doesDirectoryExist, resulting in a isAlreadyExists exception being thrown. We could do better here by calling System.Posix.getFileStatus if available. (ToDo?: what about Windows?)

• the test should check for two threads both doing createDirectoryIfExists; cleanup too.

[duncan]

Just to clarify for other readers...

createDirectoryIfMissing cannot be simultaneously atomic and sensible. The problem is that POSIX mkdir does not distinguish if a directory already exists vs if a file already exists. We want to fail in one case and return () in the other.

One way to square this circle is to test for the existence of the directory separately from the call to mkdir.

The original code did the test before. That meant it was non-atomic and a createDirectoryIfMissing raced against itself would sometimes fail with an exception about the directory already existing.

The current code does not do the test at all. That means that we return () when a file exists and this is not sensible.

The proposed code does the test afterwards. This makes the mkdir bit atomic but we can get a wrong exception if another thread does something else between the mkdir and test. The proposed code goes wrong when another thread deletes the directory in between the call to createDirectory and doesDirectoryExist and in this case throws an already-exists exception which is a bit confusing. Ideally in this scenario there would be no exception (though if it were a parent dir that was deleted then we would expect an exception when it tries to create the child directory). There are really three cases that we want to distinguish when createDirectory throws a AlreadyExistsError:

• a dir exists, in which case we return ();
• a non-directory filesystem object exists, in which case we re-throw the exception;
• no filesystem object exists, in which case we return ().

It just so happens that we can handle these three cases with just doesFileExist because we want to do the same thing in two cases that doesFileExist identifies. This also relies on the fact that doesFileExist considers all non-directory files to be files, including symlinks, sockets, fifos, etc. If that ever changes then this trick does not work. It is more explicit to use the internal withFileStatus function.

Another behavior question is if createDirectoryIfMissing should ever attempt to re-create a directory that it previously created and got subsequently deleted. The current createDirectory001 test assumes this behavior by racing removeDirectoryRecursive with createDirectoryIfMissing and expecting no exceptions. If instead we think that createDirectoryIfMissing should create each parent directory at most once then the test cannot assume never to encounter a does-not-exist exception. This is because the thread running createDirectoryIfMissing could make a parent dir, the other thread delete it and the first thread then try to make the child directory inside the now-deleted parent directory. The proposed code has this behavior.

[duncan]

Updated patch

[simonmar]

high prio because it's nearly done, I should finish this before the release.

[simonmar]

See also #2924, uncovered while testing this patch.

To merge:

Thu Dec 18 14:47:04 GMT Standard Time 2008  Duncan Coutts <duncan@haskell.org>
  * Alternative implementation of createDirectoryIfMissing

Mon Dec 22 16:41:05 GMT Standard Time 2008  Duncan Coutts <duncan@haskell.org>
  * Fix createDirectoryIfMissing to not throw if the dir got deleted
  When we call createDirectory and some file system object already exists
  we have a problem. We need to distinguish if it is a file that already
  exists or if it is a directory because in the latter case it is not an
  error. Previously we called doesDirectoryExist however that does not
  distinguish the dir not existing (due to another thread deleting it)
  and an ordinary file existing. We now use withFileStatus to throw the
  original AlreadyExistsError only if a non-directory object exists.
  So now the only time we should get a spurious exception is if another
  thread deletes the directory and puts a file in its place between our
  call to createDirectory and withFileStatus. It should now be safe to
  race createDirectoryIfMissing with itself or deleteDirectoryRecursive.

Wed Feb  4 16:33:19 GMT Standard Time 2009  Simon Marlow <marlowsd@gmail.com>
  * fix this test for the new version of createDirectoryIfMissing (#2808)
  - add another race test, for two threads both doing create;cleanup
  - ignore isDoesNotExistErrors in create

[igloo]

All 3 merged.