5

I've got the following setup:

public class SomeClass
{
  private DirectoryEntry _root;
  private DirectorySearcher _searcher;

  public SomeClass()
  {
    _root = new DirectoryEntry("ldap://bla");
    _searcher = new DirectorySearcher(_root)
      {
        PageSize = int.MaxValue,
        SizeLimit = int.MaxValue
      }
  }
}

The reason I use int.MaxValue is because in this case I know I'll exceed the defaults, but the number will never be ridiculously large, so I'm fine about that point.

But if I turn on Code Analysis and Microsoft Basic Correctness Rules it complains:

Warning 2 CA2000 : Microsoft.Reliability : In method 'SomeClass.SomeClass()', object '<>g_initLocal0' is not disposed along all exception paths. Call System.IDisposable.Dispose on object '<>g_initLocal0' before all references to it are out of scope.

It's problem is that PageSize and SizeLimit can throw exceptions, and if that happens the G__initLocal0 object doesn't get disposed (even if _searcher does get disposed). The exception they can throw is if you assign them to a negative number, which can't happen here but it still complains.

Next I set the properties outside an object intitializer with regular assignment statements, but then ReSharper complains telling me that I should be using Initializer. I could suppress ReSharper, but I like to figure a way of getting things to work without adding suppressing.

So I figured I had to catch the error and I don't like catch errors in my constructors if possible, so I created a property called Searcher like this:

private DirectorySearcher _searcher;
public DirectorySearcher Searcher
{
  get
  {
    if (_searcher != null) return _searcher;
    var searcher = new DirectorySearcher();
    try
    {
      searcher.PageSize = int.MaxValue;
      searcher.SizeLimit = int.MaxValue;
      _searcher = searcher;
    }
    catch
    {
      searcher.PageSize = 1000;
      searcher.SizeLimit = 1000;
    }
    finally
    {
      searcher.Dispose();
    }
    return _searcher;
  }
}

Now Code Analysis and everything is happy, but I'm not happy with the solution at all.

Any hints?

carlsb3rg
  • 752
  • 6
  • 14
  • 3
    Your property is going to return a disposed `DirectorySearcher` at the moment... – Jon Skeet Jun 20 '11 at 10:41
  • "...I don't like catch errors in my constructors". Why is that? Seems more acceptable then what you are doing now. – Edwin de Koning Jun 20 '11 at 10:41
  • `SomeClass` should implement `IDisposable` and clean up there. – Grant Thomas Jun 20 '11 at 10:46
  • I see that I'm returning a disposed DirectorySearcher, but the damn thing works for some reason. Even a 2nd call to Searcher works fine. I could move this code to my constructor, but the problem would still be the same... – carlsb3rg Jun 20 '11 at 10:56
  • "the damn thing works for some reason" -> I can confirm that DirectorySearcher defies all logic and does not throw any sort of exception after being disposed and then invoked to do directory searching. But why would you rely on that? You are worried about the purely esthetic comment ReSharper makes on your code, but not worried about using a disposed object? – TamaMcGlinn Dec 24 '18 at 08:10
  • Hopefully, the code generated for object initializers will be fixed in the language at some point. In the meantime, [ReSharper is fixing their bug](https://youtrack.jetbrains.com/issue/RSRP-209157) that recommended you to to break your code. In the meantime, you should disable the whole "use object initializer" recommendation. – TamaMcGlinn Dec 24 '18 at 08:14

3 Answers3

7

The problem is the compiler is effectively generating:

public class SomeClass
{
  private DirectoryEntry _root;
  private DirectorySearcher _searcher;

  public SomeClass()
  {
    _root = new DirectoryEntry("ldap://bla");

    var temp = new DirectorySearcher(_root);
    temp.PageSize = int.MaxValue;
    temp.SizeLimit = int.MaxValue;

    _searcher = temp;
  }
}

You can avoid this by not using the newer property initializer syntax for _searcher, so that you can ensure it is properly assigned to the field before you set the properties, see Object initializers in using-block generates code analysis warning CA2000.

There is a second problem here, and that is if there is an error during construction of SomeClass the calling code will not be able to dispose of SomeClass and thus cannot dispose of _root or _searcher, see Handling IDisposable in failed initializer or constructor.

Community
  • 1
  • 1
Chris Chilvers
  • 6,429
  • 3
  • 32
  • 53
4

The way you are doing things now might make everyone happy but you as it wont work. You are returning a disposed DirectorySearcher in your Searcher property.

I'd just do:

public SomeClass()
{
    _root = new DirectoryEntry("ldap://bla");

    try
    {
        _searcher = new DirectorySearcher(_root);
        _searcher.PageSize = 1000;
        _searcher.SizeLimit = 1000;
    }
    catch
    {
         if (_searcher != null)
         {
             _searcher.Dispose();
         }

         throw;
    }

}

I don't see whats wrong in using try-catch blocks in constructors.

I wouldn't recommed the property initializer syntax when constructing IDisposable objects as you cannot dispose them normally if a property initialization throws.

InBetween
  • 32,319
  • 3
  • 50
  • 90
  • Both yours and Chris' answers are good so I'll have to give it to you since you're first. – carlsb3rg Jun 21 '11 at 06:38
  • This general outline seems to work. Still can't figure out why the newer syntax won't work even when you place it in the Try/Catch logic. This is a very subtle (and initially, very annoying) error but now that I understand it I'm glad that FXCop is issuing the warning. – Quark Soup Feb 21 '15 at 16:47
0

If you need _searcher during the whole lifetime of a SomeClass instance, SomeClass should implement IDisposable and dispose _searcher within Dispose.

See http://msdn.microsoft.com/de-de/library/system.idisposable.aspx.

Stephan
  • 4,187
  • 1
  • 21
  • 33