Recursive directory deletion with WIN32

Question

I've written an application that uses the WIN32 api to create a temporarily directory hierarchy. Now, when wanting to delete the directories when shutting down the application I'm running into some problems.

So lets say I have a directory hierarchy: C:\temp\directory\subdirectory\

I'm using this recursive function

bool    Dir::deleteDirectory(std::string& directoryname, int flags)
{
    if(directoryname.at(directoryname.size()-1) !=  '\\') directoryname += '\\';

    if ((flags & CONTENTS) == CONTENTS)
    {
     WIN32_FIND_DATAA fdata;
     HANDLE dhandle;

     directoryname += "\\*";
     dhandle = FindFirstFileA(directoryname.c_str(), &fdata);

     // Loop through all the files in the main directory and delete files & make a list of directories
     while(true)
     {
      if(FindNextFileA(dhandle, &fdata))
      {
       std::string filename = fdata.cFileName;
       if(filename.compare("..") != 0)
       {
        std::string filelocation = directoryname.substr(0, directoryname.size()-2) + StringManip::reverseSlashes(filename);

        // If we've encountered a directory then recall this function for that specific folder.
        if(!isDirectory(filelocation)) DeleteFileA(filename.c_str());
        else deleteDirectory(filelocation, DIRECTORY_AND_CONTENTS);
       }
      } else if(GetLastError() == ERROR_NO_MORE_FILES)    break;
     }
     directoryname = directoryname.substr(0, directoryname.size()-2);
    }

    if ((flags & DIRECTORY) == DIRECTORY)
    {
     HANDLE DirectoryHandle;
     DirectoryHandle = CreateFileA(directoryname.c_str(),
        FILE_LIST_DIRECTORY,
        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
        NULL,
        OPEN_EXISTING,
        FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
        NULL);
     bool DeletionResult = (RemoveDirectoryA(directoryname.c_str()) != 0)?true:false;
     CloseHandle(DirectoryHandle);
     return DeletionResult;
    }

     return true;
}

This function iterates over the directory contents of the temp directory; and for each directory in the temp directory it keeps recalling itself until it's at the lowest directory; subdirectory in the example.

There are also 3 flags defined

 enum DirectoryDeletion
 {
    CONTENTS = 0x1,
    DIRECTORY = 0x2,
    DIRECTORY_AND_CONTENTS = (0x1 | 0x2)
 };

When using this function, it only removes the lowest subdirectory and I can't remove the ones higher in hierarchy because it says that the directory is not empty. When I go and look to the directory 'subdirectory' is only removed after the application ends. However, when I try to encapsulate this in a non recursive simple main application I have no problems at all with deleting the directories.

Any help and input is appreciated :)

Thanks.

Answer 1

You're not closing dhandle from all those FindFirstFile calls, so each directory has a reference to it when you try to delete it.

And, why do you need to create DirectoryHandle? It's not needed, and will probably also block the directory deletion.

When you app closes, those handles are forced close, and (I guess) the last attempted delete then succeeds.

Answer 2

Try calling FindClose to close handle returned by FindFileFileA.

Answer 3

I don't see a FindClose for your dhandle. An open handle means that the directory is still in use.

MSDN says: "When the search handle is no longer needed, close it by using the FindClose function, not CloseHandle."

(CloseHandle appears to be correct for your DirectoryHandle further down, but not for the dhandle used in the Find loop.)

Answer 4

Well, I found several bugs in this code.. here is what I found

bool Dir::deleteDirectory(std::string& directoryname, int flags)
{
 if(directoryname.at(directoryname.size()-1) !=  '\\') directoryname += '\\';

 if ((flags & CONTENTS) == CONTENTS)
 {
  WIN32_FIND_DATAA fdata;
  HANDLE dhandle;
  //BUG 1: Adding a extra \ to the directory name..
  directoryname += "*";
  dhandle = FindFirstFileA(directoryname.c_str(), &fdata);
  //BUG 2: Not checking for invalid file handle return from FindFirstFileA
  if( dhandle != INVALID_HANDLE_VALUE )
  {
      // Loop through all the files in the main directory and delete files & make a list of directories
   while(true)
   {
    if(FindNextFileA(dhandle, &fdata))
    {
     std::string     filename = fdata.cFileName;
     if(filename.compare("..") != 0)
     {
      //BUG 3: caused by BUG 1 - Removing too many characters from string.. removing 1 instead of 2
      std::string filelocation = directoryname.substr(0, directoryname.size()-1) + filename;

      // If we've encountered a directory then recall this function for that specific folder.

      //BUG 4: not really a bug, but spurious function call - we know its a directory from FindData already, use it.
      if( (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0)  
       DeleteFileA(filelocation.c_str());
      else 
       deleteDirectory(filelocation, DIRECTORY_AND_CONTENTS);
     }
    } else if(GetLastError() == ERROR_NO_MORE_FILES)    break;
   }
   directoryname = directoryname.substr(0, directoryname.size()-2);
   //BUG 5: Not closing the FileFind with FindClose - OS keeps handles to directory open.  MAIN BUG
   FindClose( dhandle );
  }
 }
 if ((flags & DIRECTORY) == DIRECTORY)
 {
  HANDLE DirectoryHandle;
  DirectoryHandle = CreateFileA(directoryname.c_str(),
   FILE_LIST_DIRECTORY,
   FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
   NULL,
   OPEN_EXISTING,
   FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
   NULL);
  //BUG 6: Not checking CreateFileA for invalid handle return.
  if( DirectoryHandle != INVALID_HANDLE_VALUE )
  {

   bool DeletionResult = (RemoveDirectoryA(directoryname.c_str()) != 0)?true:false;
   CloseHandle(DirectoryHandle);
   return DeletionResult;
  }
  else
  {
   return true;
  }
 }

 return true;
}

Answer 5

The main issue has already been answered, but here's something I noticed. Your main while loop seems a bit fragile to me...

while(true)
{
     if(FindNextFileA(dhandle, &fdata))
     {
         //...
     } else if(GetLastError() == ERROR_NO_MORE_FILES)    break;
}

This ends if FindNextFile ends because there are no more files in the directory. But what if it ever ends for some other reason? If something abnormal happens, it seems you could end up with an infinite loop.

I'd think if FindNextFile fails for any reason, then you'll want to stop the loop and start returning through the recursive calls. So I'd suggest simply removing the GetLastError test and just making it "else break;"

---

Actually, after a moment's thought I would probably just reduce it to:

while(FindNextFileA(dhandle, &fdata))
{
    //...
}

Answer 6

All of you folks are making this problem way too hard. There's a Windows API, SHFileOperation, that will do a recursive folder delete for you.

LONG DeleteDirectoryAndAllSubfolders(LPCWSTR wzDirectory)
{
    WCHAR szDir[MAX_PATH+1];  // +1 for the double null terminate
    SHFILEOPSTRUCTW fos = {0};

    StringCchCopy(szDir, MAX_PATH, wzDirectory);
    int len = lstrlenW(szDir);
    szDir[len+1] = 0; // double null terminate for SHFileOperation

    // delete the folder and everything inside
    fos.wFunc = FO_DELETE;
    fos.pFrom = szDir;
    fos.fFlags = FOF_NO_UI;
    return SHFileOperation( &fos );
}

Have a nice day.