2

In UMLs composition aggregations I tend to use the IDisposable interface/pattern to force the "Child can't exist without Parent" - requirement the composition demands:

public class Parent : IDisposable
{
    private readonly _childs;
    public IReadOnlyCollection<Child> Childs => _childs;

    public Parent()
    {
        _childs = new List<Child>();
    }

    public Child CreateChild()
    {
        var child = new Child();
        _childs.Add(child);
        return child;
    }

    public void Dispose()
    {
        foreach (var child in _childs)
            child.Dispose();
    }
}

public class Child : IDisposable
{
    internal Child() {}

    public void Dispose()
    {
         //Cleanup object, imagine an idempotent implementation
    }
}

So far, so good. But now imagine this piece of code:

var parent = new Parent();
var child = parent.CreateChild();
child.Dispose();
//At this point parent.Childs contains a disposed child object

Since I am currently facing such a situation in a library I develop, several questions come to my mind:

  • Is it okay, that parent.Childs contains an (in practise) unusable object?
    • If yes
      • would you ignore it, since it's the users own decision to prematurely dispose it?
    • If not
      • Is there some kind of best-practise on how to deal with premature disposal of child-objects in C#? My first thought was the use of a callback function/delegate on disposal of the child object to remove itself from the list of active instances, but to me that sounds rather clumsy.
      • Is there another excuse so that I can wash my hands of responsibility?

From an architectural point of view the main problem is, that IDisposable is visible to everyone being able to obtain an instance of Child. Hiding it basically means making use of OO-polymorphy and extract the ability of disposal to an invisible implementation. But for many classes in a domain model this gets an absolutely bloating factor with no additional benefit. Furthermore it inherently interprets UMLs composition aggregation as "Child can only be destroyed by Parent", which is incorrect in my opinion:

public interface IChild
{
    //Child methods
}

internal class Child : IChild, IDisposable
{
    //See implementation above
}

public class Parent : IDisposable
{
    public IReadOnlyCollection<IChild> Childs => _childs;

    public IChild CreateChild()
    {
        var child = new Child();
        _childs.Add(child);
        return child;
    }
}
Peter Wurzinger
  • 391
  • 2
  • 9
  • I would probably use `IsDisposed` event _(self implemented)_ that remove child from parent when it was disposed. – Julo Sep 26 '18 at 11:55
  • This seems like an usual way to use `IDisposable` and its unclear to everyone (without looking at the class Disposable pattern) what to expect – TheGeneral Sep 26 '18 at 11:57
  • 1
    One option is to make the interface implementation of `IDisposable` in `Child` explicit: `void IDisposable.Dispose()` - then the caller has to cast the child to `IDisposable` to be able to dispose it, i.e. `((IDisposable)child).Dispose();` – stuartd Sep 26 '18 at 11:57
  • 1
    @stuartd That wouldn't help with `using` statements, since those are still allowed. It crossed my mind too, but it seems to error prone to me. – Patrick Hofman Sep 26 '18 at 11:59
  • @PatrickHofman did not know that, thanks. – stuartd Sep 26 '18 at 11:59
  • Update after Patrick Hofman answer: Since code analysers request you dispose objects with `IDisposable` when you use them _(outside of `Parent`)_ it can also be done in another way. Make `DisposableChild` and `PublicChild` when you need to dispose resources from `Child`. – Julo Sep 26 '18 at 12:24
  • I would consider using `List>` – xmojmr Sep 26 '18 at 12:25
  • 1
    Here's an interesting question that, while about a specific implementation, is closely related to this questions: https://stackoverflow.com/questions/50912160/should-httpclient-instances-created-by-httpclientfactory-be-disposed – spender Sep 26 '18 at 12:29

1 Answers1

2

Is it okay, that parent.Childs contains an (in practice) unusable object?

Yes, but you should never touch it again. Usually you would throw a ObjectDiposedException when touching it when disposed.

My first thought was the use of a callback function/delegate on disposal of the child object to remove itself from the list of active instances, but to me that sounds rather clumsy.

Clumsy and dangerous indeed. Imagine you have a collection of financial data and suddenly some objects get removed because some developer mistakenly disposed an object. Rather throw an exception as stated above.

The question that remains is: should child objects implement IDisposable, or should they have a 'dispose' method only known and available to the parent class? That approach seems to make more sense to me.

Patrick Hofman
  • 153,850
  • 22
  • 249
  • 325
  • I take this as a vote for "ignore it, since it's the users own decision to prematurely dispose it". A Dispose-Method only known and available to the parent class is basically demostrated in the second code sample. I don't know if it's worth the additional complexity. – Peter Wurzinger Sep 26 '18 at 12:16
  • Then read again. This is a vote for: make your software robust so you can't end up in this situation. – Patrick Hofman Sep 26 '18 at 12:16
  • Uhm, I don't know where my comment would have sounded offensive to you, but it wasn't my intention doing so. Making software robust is another scope, this question basically boils down on how to translate architectural abstraction to (C#) Code. And in the scope of the question not removing a reference to a dead object is an absolutely valid alternative, imho. – Peter Wurzinger Sep 26 '18 at 12:27
  • @PeterWurzinger Uhm, I don't know where my comment would have sounded offensive to you, but it wasn't my intention doing so. ;) – Patrick Hofman Sep 26 '18 at 12:28
  • Making software robust is not another scope. It is the core of your problem. You allow something by using and exposing that interface that breaks your program. Don't allow it then and search for other options. – Patrick Hofman Sep 26 '18 at 12:29
  • Not allowing it by not exposing `IDisposable` is already discussed in the third code sample and the text block above it. In my opinion it's not a correct implementation of UMLs composition aggregation, since it makes an additional assumption. But I'm ofc highly willing to discuss that. – Peter Wurzinger Sep 26 '18 at 13:13