Why am I having problems recursively deleting directories?

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.

when I read your subject line, I thought you were trying to delete the win32 directory... — rmeador, Sep 23 '09 at 22:31

selbie · Answer 1 · 2015-02-03T02:03:41.967

13

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

edited Feb 03 '15 at 02:03

answered Sep 26 '09 at 05:37

selbie

100,020
15
103
173

2

Warning: this does not work on Vista+. Yes, the MSDN documentation says it's only "superseded" by IFileOperation, but in reality, FO_DELETE is broken on Vista and 7. – Mahmoud Al-Qudsi Feb 09 '10 at 16:09
2

Sorry, but I beg to differ. I just tried the above code out on Win7 by compiling it and calling: DeleteDirectAndAllSubfolders(L"D:\\somefolder") Works great. Now I recall when I first looked into this that it might not work on XP. But I don't see anything in the documentation that would suggest that now. It will fail if the directory is in use (such as having an open shell folder and/or dos prompt with curdir in that folder) – selbie Feb 15 '10 at 08:20

score 8 · Accepted Answer · edited Nov 08 '12 at 17:42

8

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.

edited Nov 08 '12 at 17:42

Nik Reiman

39,067
29
104
160

answered Sep 23 '09 at 22:14

DougN

4,407
11
56
81

Indeed, forget the FindClose. Thank you! – Charles Sep 23 '09 at 22:22

score 7 · Answer 3 · answered Aug 28 '11 at 21:25

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)

score 6 · Answer 4 · answered Sep 23 '09 at 23:04

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

score 1 · Answer 5 · answered Sep 23 '09 at 22:18

1

Try calling FindClose to close handle returned by FindFileFileA.

answered Sep 23 '09 at 22:18

jdigital

11,926
4
34
51

score 1 · Answer 6 · answered Sep 23 '09 at 22:21

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.)

score 0 · Answer 7 · answered Sep 23 '09 at 23:26

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

Why am I having problems recursively deleting directories?

7 Answers7

Linked