Reputation: 2661

Why am I having problems recursively deleting directories?

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.

Upvotes: 4

Answers (7)

selbie

Reputation: 104484

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 );
}

Upvotes: 13

DougN

Reputation: 4607

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 your app closes, those handles are forced close, and (I guess) the last attempted delete then succeeds.

Upvotes: 8

Malcolm McCaffery

Reputation: 71

SHFileOperations works great on Windows 7. In fact in the IFileOperation documentation says

IFileOperation can only be applied in a single-threaded apartment (STA) situation. It cannot be used for a multithreaded apartment (MTA) situation. For MTA, you still must use SHFileOperation.

However my issue with SHFileOperations is it doesn't seem to support paths longer than 260 characters, and does not support \?\ prefix for long filenames.

This is a real pain....but a recursive function is still needed if you want ability to handle paths longer than 260 characters (Which NTFS supports - but not Windows Explorer, command prompt commands etc)

Upvotes: 7

TheUndeadFish

Reputation: 8171

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))
{
    //...
}

Upvotes: 0

Jay Kramer

Reputation: 324

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;
}

Upvotes: 6