CODEWITHSUNDEEP
c#arraysforeachout-of-memorydispose
Chris
Chris

Reputation: 442

Can't get .Dispose() to work in a foreach loop

So I've got this foreach loop here

foreach (string file in condensedFilesList)
{
    Image imgToAdd;
    imgToAdd = Image.FromFile(file);

    if (imgToAdd.Width < 1920 || imgToAdd.Height < 1080)
    {
        //neither of the commented out lines worked when placed here
        //imgToAdd = null;
        //imgToAdd.Dispose();
        condensedFilesList.Remove(file);
    }
    else
    {
        //neither of the commented out lines worked when placed here
        //imgToAdd = null;
        //imgToAdd.Dispose();
        continue;
    }
}

It contains a list of file paths pointing to .jpg images. About 80 of them of various sizes. I need the list to go through each image, check to see if its resolution is 1920*1080, and if it is not, remove that file path pointer from the array.

Right now it's going through, setting the image to review in the imgToAdd variable, then if the width property or height property don't match up that item is to be removed. This works for the first entry. It's resolution doesn't fit the bill, and my array will drop from 80 to 79 entries.

But I can't get my imgToAdd variable to empty out so I can assign it a new filePath. I keep running into a OutOfMemoryException. I've tried running .Dispose(), setting it equal to null, and I can't get it to actually empty itself of it's resources.

In the debugger .Dispose() causes imgToAdd to have a long list of errors in place of values when you inspect the element. All of it's properties are there, but valueless and replaced with errors. If I set it = to null, it works, and on the next iteration, imgToAdd = null. Buuuuut, I'm still getting OutOfMemoryException when it tries to assign a new filePath to the variable.

So I have no clue what's up with that. I'm hoping someone else could maybe point out what I'm doing wrong, I can't see it.

EDIT2 :

I'm just going to overwrite this edit space, if people want to check the evolution of the function as I update, hit up the edit history. I tried using a using(){} statement like @dlatikay recommended, and have it writing to a new list., but unfortunately I'm still getting the OutOfMemoryException. Here's the function right 

        var tempList = new List<string>();

        foreach (string file in condensedFilesList)
        {
            using (Image imgToAdd = Image.FromFile(file))
            {
                if (imgToAdd.Width < 1920 || imgToAdd.Height < 1080)
                {
                    continue;
                }
                else
                {
                    tempList.Add(file);
                }
            }
        }

        condensedFilesList = tempList;

Upvotes: 4

Views: 1263

Answers (3)

Cee McSharpface
Cee McSharpface

Reputation: 8725

Use using. And write the result to a new list, so you won't be modifying the source list while enumerating it:

var finalList = new List<string>();
foreach (string file in condensedFilesList)
{
    using(var imgToAdd = Image.FromFile(file))
    {
        if (imgToAdd.Width < 1920 || imgToAdd.Height < 1080)
        {
            /* omit */
        }
        else
        {
            finalList.Add(file);
        }
    }
}

No need to assign null, or to explicitely call Dispose(). I recommend to add try..catch, not all image files are valid.

Upvotes: 4

Richard Schneider
Richard Schneider

Reputation: 35477

You cannot remove an item from a list, when the list is being enumerated.

for (int i = condensedFilesLists.Length - 1; 0 <= i; --i)
{
    using (var image = Image.FromFile(condensedFilesLists[i]))
    {
        if (image.Width < 1920 || image.Height < 1080)
        {
           condensedFilesList.Remove(file);
        }
    }
 }

Upvotes: 0

juharr
juharr

Reputation: 32296

On top of setting a variable to null before you attempt to call a method on it which is the beginning of your problems you'll also get run time errors about modifying a collection that you are iterating. Here's how I would write that code to make it work properly.

foreach (string file in condensedFilesList.ToList())
{
    using(var imgToAdd = Image.FromFile(file))
    {
        if (imgToAdd.Width < 1920 || imgToAdd.Height < 1080)
        {
            condensedFilesList.Remove(file);
        }
    }
}

The ToList will create a separate collection to iterate over so you can safely use condensedFilesList.Remove. By putting the imgToAdd into a using statement you no longer need to worry about calling Dispose as it will be called at the end of the statement even if an exception occurs.

Upvotes: 4

Related Questions