CODEWITHSUNDEEP
c#idisposableconstructor
Matt Enright
Matt Enright

Reputation: 7484

Passing IDisposable objects through constructor chains

I've got a small hierarchy of objects that in general gets constructed from data in a Stream, but for some particular subclasses, can be synthesized from a simpler argument list. In chaining the constructors from the subclasses, I'm running into an issue with ensuring the disposal of the synthesized stream that the base class constructor needs. Its not escaped me that the use of IDisposable objects this way is possibly just dirty pool (plz advise?) for reasons I've not considered, but, this issue aside, it seems fairly straightforward (and good encapsulation).

Codes:

abstract class Node {
    protected Node (Stream raw)
    {
        // calculate/generate some base class properties
    }
}
class FilesystemNode : Node {
    public FilesystemNode (FileStream fs)
        : base (fs)
    {
        // all good here; disposing of fs not our responsibility
    }
}
class CompositeNode : Node {
    public CompositeNode (IEnumerable some_stuff)
        : base (GenerateRaw (some_stuff))
    {
        // rogue stream from GenerateRaw now loose in the wild!
    }

    static Stream GenerateRaw (IEnumerable some_stuff)
    {
        var content = new MemoryStream ();
        // molest elements of some_stuff into proper format, write to stream
        content.Seek (0, SeekOrigin.Begin);
        return content;
    }
}

I realize that not disposing of a MemoryStream is not exactly a world-stopping case of bad CLR citizenship, but it still gives me the heebie-jeebies (not to mention that I may not always be using a MemoryStream for other subtypes). It's not in scope, so I can't explicitly Dispose () it later in the constructor, and adding a using statement in GenerateRaw () is self-defeating since I need the stream returned.

Is there a better way to do this?

Preemptive strikes:

Upvotes: 6

Views: 1469

Answers (4)

Simon Buchan
Simon Buchan

Reputation: 13245

For this simple example, I would use a InitializeFrom(Stream s) method:

abstract class Node
{
    public Node(Stream stream) { InitializeFrom(stream); }
    protected Node() { }
    protected void InitializeFrom(Stream stream);
}

class FilesystemNode
{
    public FilesystemNode(FileStream stream) : base(stream) {}
}

class CompositeNode
{
    public CompositeNode(IEnumerable values) : base()
    {
        using (var stream = new MemoryStream())
        {
            // init stream
            InitializeFrom(stream);
        }
    }
}

Making it virtual if you have a deeper hierarchy. I tend to find code like this a bit hard to keep track of though, and use the pattern I've seen in full library/framework codes: splitting into plain objects (preferably immutable and ignorant of what creates them, eg from their members only) and the Readers (or Factories if the data is not coming from a Stream) that create them, but a middle ground is a static reader method:

abstract class Node
{
    NodeKind kind;
    public Node(NodeKind kind) { this.kind = kind; }
    public NodeKind Kind { get { return kind; } }

    static Node ReadFrom(Stream stream);
}

class FilesystemNode : Node
{
    string filename;
    public FilesystemNode(string filename) : Node(NodeKind.Filesystem)
    {
        this.filename = filename;
    }
    public string Filename { get { return filename; } }

    static FilesystemNode ReadFrom(FileStream stream);
}

class CompositeNode : Node
{
    Node[] values;
    // I'm assuming IEnumerable<Node> here, but you can store whatever.
    public CompositeNode(IEnumerable<Node> values) : Node(NodeKind.Composite)
    {
        this.values = values.ToArray();
    }
    public IEnumerable<Node> { get { return filename; } }
}

Upvotes: 0

Nicole Calinoiu
Nicole Calinoiu

Reputation: 20992

You may want to consider passing instructions regarding disposition as/within a separate argument of the constructor that accepts an IDisposable. This is the approach taken by XmlReader.Create, which accepts an XmlReaderSettings parameter whose CloseInput property determines whether the underlying data source is disposed when the created XmlReader is eventually disposed.

Upvotes: 2

Marc Gravell
Marc Gravell

Reputation: 1062895

CompositeNode created the stream - CompositeNode has the responsibility unless some other code explicitly states it'll take that on. One option here would be for the base-constructor to allow this via a protected overload, but the ordering means that it would be hard to know exactly when it is safe to dispose it. A virtual method would allow you to do this, but you shouldn't really call virtual methods in constructors.

I wonder if refactoring so that there is an initialize (Load) method (that you call separately to construction) would be better. Perhaps a protected virtual method, and expose it via a public static method.

Upvotes: 4

Neil Barnwell
Neil Barnwell

Reputation: 42125

The main rule of thumb is that the code that creates an instance of a disposable object should dispose it. If you have an IDisposable object passed into a method, you should use it for what you need, and leave it alone.

A good way to ensure you always do this is to use the using ([IDisposable object]) { ... } pattern, which will call dispose on the object automatically when the scope is completed.

Upvotes: -1

Related Questions