Racing Directory Creation is OK So Long As the Directory Exists #2810
Conversation
…existance just before you try to make it but after you've checked and found it wasnt there
|
wow! Kevins 1 : 0 bug |
… folder removal was crashing until I added some error handling to the directory_entry() ctor.
…ed doesn't exist.
| } | ||
|
|
||
| inline bool remove_all(const path& p) { | ||
| inline std::uintmax_t remove_all(const path& p) { |
There was a problem hiding this comment.
Modified prototype/impl to match C++17 ::remove_all().
|
Made a few comments, annoyingly largely around syntax and formatting. A few of these I could have expected to be caught by |
|
@mookerji I think the things you're flagging are only caught by |
…g line" using a condition variable so the threads would be as concurrent as possible.
| // it can occur that another thread is removing the same file | ||
| // object at the same time. Occasionally it will occur that | ||
| // while one thread is deleting (thread 1), the other thread | ||
| // (thread 2) will receive -1 from ::remove() and (errno==ENOENT | ||
| // or errno==EINVAL). And yet, the file still exists until thread 1 | ||
| // is finished deleting. In this exact moment, if thread 2 calls | ||
| // stat() on the file object in question it will get 0 which means | ||
| // "file object exists". We kinda want thread 2 to wait for the | ||
| // file object to truly go out of existence. Hence, this spin loop. |
There was a problem hiding this comment.
while i get that this is a nice feature i really dont think it should be the filesystem libraries concern if you write code that is racing to delete the same paths. that synchronization should be handled in the calling application. indeed i had a look at both the boost implementation as well as another filesystem implementation (https://github.com/gulrak/filesystem) and neither of them implement such a tactic:
https://github.com/gulrak/filesystem/blob/de57485877865156370f5e9878592293645b207e/include/ghc/filesystem.hpp#L4396
https://github.com/boostorg/filesystem/blob/develop/src/operations.cpp#L229
@ktatterso what was the issue that triggered you looking into this anyway? i've kind of lost the plot at this point 😄 i cant think of any multithreaded calls to remove. in mjolnir at the beginning of the process we remove_all existing tile directories in the main thread... what am i missing here?
There was a problem hiding this comment.
yeah it seems like its not something we really need to worry about at this time. the only call was the one that i remembered off the top of my head, the rest are all in tests which definitely shouldnt race because they definitely shouldnt be using the same tile directories:
now i get that you could be racing at the process level but again should filesystem really be the one protecting against that? i guess since it has such a small impact its probably ok but its again likely a deviation from the standard implimentation. i'm ok with whichever direction you want to go. want to just ship what we've got?
There was a problem hiding this comment.
Multithreaded folder creation motivated the creation of the unit test. Deciding to test multithreaded directory removal was simply my idea. I figured if we want to be robust in multithreaded directory creation we would also want to be robust in multithreaded directory removal. In a sense, valhalla's filesystem::remove_all() is the "calling application" - so imo we should take responsibility for it being robust.
In my mind, I wrote a test that should work (threads concurrently calling filesystem::remove_all()). It exposed an issue and I've resolved the issue. No need to wait for a customer to find this issue, we can fix it now.
|
@ktatterso can you put a ship on this so we can merge it when the build finishes? |
|
As requested I switched the EXPECT_* macros to ASSERT_*. Then I merged with master. And although the pipeline is still running I've approved the PR. |
Through @ktatterso 's tireless effort we have finally found the underlying (at least i think so) issue causing tile building to sometimes fail. Let me explain..
When we are in the first stage of tile building we will spawn a bunch of threads to build tiles at the same time. These threads are all creating the directory structure as defined in GraphTile.h/cc. If there are enough tiles to be created and enough threads its possible that they are both competing to create the same directories at the same time. Lets look at the filename patterns structure:
Now the level determins how many subdirectories you need to make. In practice there are is at least one 3 digit sub directory (for levels 0 and 1) and up to two 3 digit sub directories (for levels 2 and 3). @ktatterso and i theorized that something was clashing when trying to make directories for these tiles and we were right, but at first it didnt seem like it should matter.
Assume two threads are trying to write tiles in the same directory, ie they both race to create the same directory tree. The thing about the directory creation is it doesnt happen all in one go, each sub directory is created one at a time. If any one of those creations fails the function returns false signaling it couldnt make the directory. At first this seems ok, surely the other thread succeded which is why this thread failed BUUUUTTT... you can have two threads trying to write tiles that look like this:
Here we have the case that thread 1 races thread 2 for the creation of the subdirectory
452and wins. Thread 2 then gives up up completely and says well I couldn't do it. But no one else tries to make the next sub directory for thread 2, the999part. Because of this when thread 2 tries to open up the file for writing it cant because the deepest subdirectory doesnt exist.The good news is the fix should be easy. We can simply ignore when we fail to make the directory if it already existed and move on with the rest of the directory creation as normal. I've forgone writing a test for this as its super hard to do with the whole threading concerns.