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

Question

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;

FYI You cannot modify a collection that you are iterating. You either need to create a temporary collection to iterate or you need to do a `for` loop that starts at the end and works to the front of the list. — juharr, Dec 08 '16 at 20:35
@AlexKrupka No need to set the variable to `null` at all. In fact it would be better to put it in a `using` statement instead. — juharr, Dec 08 '16 at 20:36
1. the state-of-the-art way to work with the `IDisposable` pattern is `using() { }` 2. @MethodMan why cast? `System.Drawing.Image` is `IDisposable`. — Cee McSharpface, Dec 08 '16 at 20:39
do you get the OutOfMemoryException always on the same file? I saw GDI+ throw "false-positive" `OutOfMemoryException`s on corrupt files. or on open files. or on files with a pixel format not supported: http://stackoverflow.com/questions/3848132/out-of-memory-image-fromfile — Cee McSharpface, Dec 08 '16 at 20:56
@dlatikay after reading that, I took your advice to use a try catch block to see if that could be the issue, must have been. The function ended up running after using a try catch block! I've added the final working form to the body of the question! — Chris, Dec 08 '16 at 21:16
If you want to provide your own answer, then post your own answer, don't edit it into the question. — Servy, Dec 08 '16 at 21:19

score 4 · Answer 1 · answered Dec 08 '16 at 20:41

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.

from experience, this ToList() trick should go with a comment. I saw it removed too often when code was revisited after a while, developers thinking along the lines of, "it is already a list, no need for this, *remove*, check-in, oops." — Cee McSharpface, Dec 08 '16 at 20:47

Cee McSharpface · Accepted Answer · 2016-12-08T22:33:37.900

4

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.

edited Dec 08 '16 at 22:33

answered Dec 08 '16 at 20:42

Cee McSharpface

8,493
3
36
77

Hey man, just gave this a shot still getting that memory error, I've put the most recent version of the function up in the edit section. Any ideas? /: – Chris Dec 08 '16 at 20:56
see my last comment on OP – Cee McSharpface Dec 08 '16 at 20:56

score 0 · Answer 3 · answered Dec 08 '16 at 20:42

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

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

3 Answers3