1

I have linq with Select where IDisposable objects are created. Afterward there is a filter Where and that leads to some object never disposed.

Here is a repro:

class Program
{
    static void Main(string[] args)
    {
        var results = "1234567890"
            .Select(o => new Test(o))
            .Where(o => o.Value > '3' && o.Value < '7')
            .ToList();

        // do something with results
        // ...

        // dispose
        foreach (var result in results)
            result.Dispose();
    }
}

class Test : IDisposable
{
    public char Value { get; }
    public Test(char value)
    {
        Value = value;
        Console.WriteLine($"{Value}");
    }
    public void Dispose() => Console.WriteLine($"{Value} disposed");
}

Output:

1
2
3
4
5
6
7
8
9
0
4 disposed
5 disposed
6 disposed

The problem:

As you can see there are 1, 2, 3, 7, 8, 9, 0 which are created and never disposed.

My solution:

I can move Where condition inside Select, but then I need to apply ugly "return null + Where not null" workaround:

var results = "1234567890".Select(o =>
{
    if (o > '3' && o < '7')
        return new Test(o);
    return null;
}).Where(o => o != null).ToList();

Output:

4
5
6
4 disposed
5 disposed
6 disposed

Is there a better (more elegant) way?

My workaround apart from being ugly has a problem if Select is inside some library method returning IEnumerable<T> which I can not change. How to apply Where without leakages?

3
  • Your solution is not a working workaround if your comment is true: "Unfortunately in my case I can't exchange Select and Where, because Where uses properties of object returned by Select" In your workaround you're not using those properties. Nov 28, 2018 at 13:49
  • @Rango, yep, the reason of question. I just wanted to post some solution to prevent blaming in doing nothing. Right now I decide to call Dispose() in Where as a temp workaround (thanks to Dennis) , while waiting/looking for a better solution.
    – Sinatr
    Nov 28, 2018 at 14:29
  • The class should not have a IDisposable property which is necessary to check if an instance is relevant. Instead it should have a non-disposable property. This property should either be initialized from outside(constructor/property) or by calling a method that uses a disposable(for example a connection). It's too abstract here but i guess you can separate it from the model. If the constructor or method uses the disposable they are responsible to dispose it there. Nov 28, 2018 at 14:32

5 Answers 5

1

As you can see there are 1, 2, 3, 7, 8, 9, 0 which are created and never disposed

This is because Where needs to create Test instance for each Select call to test result against condition.

After filtering, results contains subset of created objects => your code disposes only this subset items.

Is there a better (more elegant) way?

The only LINQ way is to materialize initial enumerable into List<Test> (or array) before filtering, and dispose list items:

        var results = "1234567890"
            .Select(o => new Test(o))
            .ToList();

        var filteredResults = results
            .Where(o => o.Value > '3' && o.Value < '7')
            .ToList();

        // do something with FILTERED results
        // ...

        // dispose
        foreach (var result in results)
            result.Dispose();

But if there are some reasons to Dispose unnecessary results ASAP, and you can't modify code, that produces enumerable, just don't use LINQ. Write a regular foreach:

        var enumerable = "1234567890"
            .Select(o => new Test(o));

        var results = new List<Test>();

        foreach (var item in enumerable)
        {
            if (!(item.Value > '3' && item.Value < '7'))
            {
                item.Dispose();
            }
            else
            {
                results.Add(item);
            }
        }

        // do something with results
        // ...

        // dispose
        foreach (var result in results)
            result.Dispose();
4
  • Where uses property, so it has to create the instance. Hmm.. are you saying what Where can/should also dispose discarded items? I haven't thought about it. It feels a bit hackish. Is it ok to call o.Dispose() inside Where?
    – Sinatr
    Nov 28, 2018 at 10:47
  • No, it's not OK. Do not do this. Delegate inside Where must test the condition, that's all. Any side effect (dispose in your case) will be unexpected behavior. To support such of code will be headache.
    – Dennis
    Nov 28, 2018 at 10:51
  • "unexpected behavior" - exactly my thoughts. Noone would expect some code to modify object in the linq methods. Filtering inside Select is ok, but calling dispose inside following up Where is indeed not expected. Though it will work, doesn't required passing null workaround and doesn't require modifying code containing Select... which is a solution to me. So why "do not do this"? Are there more reasons than just "headache"?
    – Sinatr
    Nov 28, 2018 at 10:55
  • 1
    @Sinatr: anyone else who will support this code will curse you. :) Seriously, this will be the code, that is hard to maintain. If you can't modify code, that returns enumerable, why to use tool (LINQ), that doesn't fit your needs? Write a foreach loop, which will enumerate initial sequence, test each item for condition, dispose it, if test failed, and add it to result list, if test passed. Side effects are OK and expected when using foreach. If you can, modify code, that produces enumerable - it shouldn't create items, if condition test failed.
    – Dennis
    Nov 28, 2018 at 11:06
0

You can first filter your string with char, then select with desired type:

var results = "1234567890".Where(ch => ch > '3' && ch < '7').Select(s => new Test(s)).ToList();

foreach (var item in results)
{
    item.Dispose();
}
0

You need to remember all items:

var allItems= "1234567890"
        .Select(o => new Test(o)).ToArray();
var result = allItems.Where(o => o.Value > '3' && o.Value < '7')
        .ToList();

than dispose allItems:

foreach (var result in allItems)
        result.Dispose();
4
  • 2
    This is interesting and even worse than what OP has. It will instantiate the objects twice, so still has a potential memory leak. If you'd test it you'd see that the same value is printed twice. Nov 28, 2018 at 10:16
  • I forgot adding .ToArray() :) Now I think it should be ok. Nov 28, 2018 at 10:27
  • Since there is a reason why OP made the Test class an IDisposable (probably something like holding/locking some resource directly or indirectly), it would probably also be a good idea to create only such IDisposables that are going to be used further. Also, your approach doesn't allow for keeping the useful Test objects around (for whatever purpose) while discarding the unused/wasted Test objects immediately...
    – user2819245
    Nov 28, 2018 at 10:30
  • OP states that his Select is in some library method, so I assume he has list of IDisposable objects and filter them not source string. Nov 28, 2018 at 10:35
0

After getting the items from the library, you can store them in an array. You can filter the array, do something with the relevant items and use the array to dispose all items:

var items = GetItemsFromLibrary().ToArray();
try
{
  var relevantItems = items.Where(o => o.Value > '3' && o.Value < '7');
  // Do something with relevant items
  // ...
}
finally
{
  // Dispose all items
  foreach (var item in items)
    item.Dispose();
}

ToArray stores the items in an array so that you will work on the same items again when disposing them instead of querying the library again which might result in new items being returned.

If you are concerned about running out of memory, you could use the following (ugly) approach to throw away the unnecessary items as early as possible. This might be an advantage, but I suspect not a very big one in comparison to storing all items with ToArray.

var items = GetItemsFromLibrary()
  .Where(o => {
           if (o.Value > '3' && o.Value < '7')
             return true;
           // Object not relevant in this case
           o.Dispose()
           return false;
         })
  .ToArray();
try
{
  // Do something with relevant items
  // ...
}
finally
{
  // Dispose all items
  foreach (var item in items)
    item.Dispose();
}

Even when disposing the unnecessary items in the Where clause, the object was already instantiated in the libary. So your best option would be to change the libary to only return the relevant items.

4
  • That's a possibility, but what if there are ziliions of items, ToArray will cause OOM. I'd like to have a solution to only instantiate filtered items.
    – Sinatr
    Nov 28, 2018 at 10:12
  • 2
    @Sinatr: if there are zillion of items you should not instantiate them all. Find a way to filter them without creating an instance. Why are they disposable at all? Nov 28, 2018 at 10:28
  • @Rango, well, I have to create instance of every item, to be able to filter them later (using properties of created object). The Select occurs elsewhere, I could theoretically pass delegate with filter Func<>, but I prefer not to alter code there. They are disposable because one of property is.
    – Sinatr
    Nov 28, 2018 at 10:42
  • @Rango, if I could avoid creating IDisposable instance, then I wouldn't ask this question.
    – Sinatr
    Nov 28, 2018 at 10:51
0

As others have already stated, you're creating objects and then throwing away the references to them which means Dispose() can not be called.

These objects will eventually be collected by GC at some point in the future, but the GC does not automatically call Dispose() for you without a little help.

If that's the pattern you really need, you could implement a finalizer on your disposable object to call Dispose() for you when GC kicks in.

class Program
{
    static void Main(string[] args)
    {
        var results = "1234567890"
                      .Select(o => new Test(o))
                      .Where(o => o.Value > '3' && o.Value < '7')
                      .ToList();

        // do something with results
        // ...

        // dispose
        foreach (var result in results)
            result.Dispose();

        // Force GC to prove dispose called...
        GC.Collect();

        Console.ReadLine();
    }
}
class Test : IDisposable
{
    public char Value { get; }
    public Test(char value)
    {
        Value = value;
        Console.WriteLine($"{Value}");
    }
    public void Dispose() => Console.WriteLine($"{Value} disposed");

    ~Test()
    {
        Dispose();
    }
}
3
  • Have you tried it? Adding finalizer doesn't do anything. Forcing GC will only dispose filtered results (4, 5, 6) again.
    – Sinatr
    Nov 28, 2018 at 10:22
  • Run my example with the GC.collect. It disposes all objects. Or, at least, it did when I ran it.
    – MarcE
    Nov 28, 2018 at 10:28
  • The only reason that is there is to prove the behaviour, I'm not suggesting GC.Collect is a solution. I''m just using it to prove that the finalizer is being called
    – MarcE
    Nov 28, 2018 at 10:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.