Is returning an object that implements IDisposable a good idea or not?

My scenario is this.

I am using System.DirectoryServices.AccountManagement for dealing with AD users and groups. My Main method (this is a console app for now) calls into a method that returns PrincipalSearchResult<Principal>. Both objects implement IDisposable.

If I return this, how can I ensure I can dispose of all these disposable objects?

class Searcher
{    
    private PrincipalSearchResult<Principal> SearchForObjects(string searchString)
    {
        PrincipalContext ctx = null;
        PrincipalSearcher principalSearcher = null;
        Principal principal = null;

        ctx = new Pri开发者_高级运维ncipalContext(ContextType.Domain, "blah", "dc=blah,dc=com");

        principal = new GroupPrincipal(ctx) { Name = searchString };
        principalSearcher = new PrincipalSearcher { QueryFilter = principal };

        return principalSearcher.FindAll();
    }
}

static void Main(string[] args)
{
    Searchers searchers = new Searchers();

    PrincipalSearchResult<Principal> theGroups = searchers.SearchForGroupsMatching("some*");

    foreach (GroupPrincipal group in theGroups)
    {
        Console.WriteLine(group.DisplayName);
        // do stuff...
    }
}

Is passing back the PrincipalSearchResult<Principal> a really bad idea, for reasons relating to unmanged object disposal?

Am I better off creating a managed proxy object?

I imagine that if I'm only caring about reading a subset of properties, it may be "better" to create a custom object containing only these properties. When writing back, in this case to an AD group, all I really need to pass into a method are the changed properties and the key. This would allow me to constrain the creation of unmanaged objects to one scope. Or is all of this unnecessary and more trouble than it's worth? My apologies for the semi-scattered stream of consciousness...

---

No it is not a bad idea. There are a lot examples in the .NET framework, that do this, e.g. SqlClient.ExecuteReader.

However, you need to document that the user needs to dispose the object as soon as he is finished using it.

You would do it like this:

using(PrincipalSearchResult<Principal> theGroups = 
      searchers.SearchForGroupsMatching("some*"))
{
    foreach (GroupPrincipal group in theGroups)
    {
        Console.WriteLine(group.DisplayName);
        // do stuff...
    }
}

---

It's not wrong to return a type that implements IDisposable, but it's inconvenient.

If you know that your caller doesn't need anything over and above a few simple properties, then there's a lot to be said for creating and returning a proxy object - this can simplify the caller's code and guarantee that the unmanaged resources are disposed properly.

In your case, if you really only want the DisplayName property, then I'd definitely create a proxy object. Then you don't need to worry at all about the lifetime of the PrincipalSearchResult<Principal>, and, more critically, any connections or whathaveyou that this object keeps open.

This also simplifies testing: you can mock / stub out the method call that returns the proxy quite easily, as the proxy object is yours.

However, if your caller's code will use methods that require the full PrincipalSearchResult<Principal> then a proxy will clearly not do.

---

Returning an object that implements IDisposable is a pattern which is simultaneously icky, useful, and in some sense unavoidable. Having a factory supply the newly-created item only as a return value creates the danger that an exception might be thrown after the object has performed operations requiring cleanup, but before it has been stored in a place that will still exist when the exception is caught.

A safer pattern would be to pass by reference a place for a factory to store the object being created. Following that pattern, if the factory stores a reference to a new object after it has been constructed well enough to tolerate Dispose, but before it has done any operations requiring cleanup, code which calls the factory could call Dispose on the newly-created object if an exception gets thrown any time after that.

Indeed, I would consider the latter style of object construction to be vastly superior to the former except for two factors:

1. The "using" construct, which allows a nice clear syntax to automate cleanup code, wouldn't really work with that style of factory (one could kludge it, but not in a way that would allow "using" to clean up the partially-constructed object if the constructor throws).
2. The normal way of calling a constructor directly gives the new instance to the calling code as a return value, which disappears if the constructor throws an exception. Every object creation eventually boils down to a call to "new()", so the only way to completely avoid having a function return value be the only existing reference to an IDisposable is to not have a class expose any public constructors, and have all its private and protected constructors accept a reference parameter in which to smuggle out the new object instance.

Having functions returning the only extant references to newly-constructed instances of IDisposable isn't wonderful, but it's a pretty common pattern since--as yet--neither c# nor .net really offers anything better.

---

The calling code should be written with a using block and that will ensure Dispose is called. IDisposable is the way to control when disposal of resources happen, so returning an object that implements IDisposable is a standard idiom.