Is this pattern OK?

[richard.markiewicz]

Hi Gurus,

Working with a .NET 2 C# web application. Using
System.DirectoryServices a lot to read information from AD. There
seemed to be several bugs in the .NET 1.1 implementation of that DLL
and out of habit I now very aggressively dispose of objects when I'm
done with them.

Is there anything wrong with this pattern:

DirectoryEntry objADAM = null;
DirectorySearcher objSearchADAM = null;
SearchResultCollection objSearchResults = null;

try
{
objADAM = new DirectoryEntry(ldap, admin, pass,
AuthenticationTypes.Secure);
objSearchADAM = new DirectorySearcher(objADAM);
objSearchResults = objSearchADAM.FindAll();

//Do something with the search results

objSearchResults.Dispose();
}
catch(Exception ex)
{
//Handle any errors
}
finally
{
objSearchADAM.Dispose();
objADAM.Dispose();
objSearchResults.Dispose();
}

The reason I ask is that this exact pattern exists in one of our web
applications that we recently updated from .NET 1.1. to .NET 2.0. In
the last week it has been randomly throwing null reference exceptions.
I cannot recreate what is causing the exception, but it can be
resolved by recycling the application in IIS. The only change to the
server in the last week is application of KB943485 and KB941644.

Looking at the stack trace indicates the null reference exception is
coming on the line:

objSearchResults.Dispose();

Within the finally{} block. So I assume I should either remove the
first call to Dispose(), and have everything in the finally{} block or
change the finally{} block to something like:

if(objSearchResults != null)
objSearchResults.Dispose();

The thing I can't work out is why this only started happening in the
last week!

Many thanks for any advice, Richard

 

[Peter Duniho]

[...]
Looking at the stack trace indicates the null reference exception is
coming on the line:

objSearchResults.Dispose();

Is the variable "objSearchResults" null when you try to execute that line?

If so, then I'd say yes...there's something wrong with this "pattern",
_somewhere_. You shouldn't be calling methods on null references, not
even Dispose().

Now, I don't see anything in the docs for DirectorySearcher.FindAll() that
suggests it should ever return a null value. So, if the exception had
been on the first instance of that line, within the "try" part of your
code, that would have seemed like a bug in the DirectorySearcher class.

However, since it's in the "finally" part of your code, then I suspect
you're getting an exception before the variable can be initialized to a
non-null value, and so of course you get an exception when you try to call
Dispose() on the null reference.

Within the finally{} block. So I assume I should either remove the
first call to Dispose(), and have everything in the finally{} block or
change the finally{} block to something like:

if(objSearchResults != null)
objSearchResults.Dispose();

You should remove the first call to Dispose() _and_ change the "finally"
block to check the variable's value for null before calling Dispose().
You should similarly check _all_ of your objects for null before calling
Dispose() on them, since you have no way to guarantee that they are
non-null when entering the "finally" block.

The thing I can't work out is why this only started happening in the
last week!

Sounds like you're getting a new exception that didn't happen before. The
bug was always in the code, but you just never hit it. As for what the
exception might be and why it's just now starting to happen, who knows?
You may just have been lucky, or it may be that some update to the system
has changed the behavior slightly, or it could be something else.

But the code was wrong in the first place, so the first thing to do is fix
it.

Pete

 

[Ben Voigt [C++ MVP]]

Hi Gurus,

Working with a .NET 2 C# web application. Using
System.DirectoryServices a lot to read information from AD. There
seemed to be several bugs in the .NET 1.1 implementation of that DLL
and out of habit I now very aggressively dispose of objects when I'm
done with them.

Is there anything wrong with this pattern:

DirectoryEntry objADAM = null;
DirectorySearcher objSearchADAM = null;
SearchResultCollection objSearchResults = null;

try
{
objADAM = new DirectoryEntry(ldap, admin, pass,
AuthenticationTypes.Secure);
objSearchADAM = new DirectorySearcher(objADAM);
objSearchResults = objSearchADAM.FindAll();

//Do something with the search results

objSearchResults.Dispose();
}
catch(Exception ex)
{
//Handle any errors
}
finally
{
objSearchADAM.Dispose();
objADAM.Dispose();
objSearchResults.Dispose();
}

The reason I ask is that this exact pattern exists in one of our web
applications that we recently updated from .NET 1.1. to .NET 2.0. In
the last week it has been randomly throwing null reference exceptions.
I cannot recreate what is causing the exception, but it can be
resolved by recycling the application in IIS. The only change to the
server in the last week is application of KB943485 and KB941644.

Looking at the stack trace indicates the null reference exception is
coming on the line:

objSearchResults.Dispose();

Within the finally{} block. So I assume I should either remove the
first call to Dispose(), and have everything in the finally{} block or
change the finally{} block to something like:

if(objSearchResults != null)
objSearchResults.Dispose();

What you ought to do instead is convert try / finally { Dispose(); } into
using blocks, which automatically handle the null case.

 

[richard.markiewicz]

What you ought to do instead is convert try / finally { Dispose(); } into
using blocks, which automatically handle the null case.

Thank you both for your input Pete and Ben.

What I have done is go ahead as Ben suggested and just replaced the
finally{} block with a using statement on all my DirectoryEntries,
DirectorySearchers and SearchResultCollections. It's certainly easier
to see what's going on that way.

Hopefully this will resolve the issue

I see that even for the latest versions of the Framework, MS still
say:

"Due to implementation restrictions, the SearchResultCollection class
cannot release all of its unmanaged resources when it is garbage
collected. To prevent a memory leak, you must call the Dispose method
when the SearchResultCollection object is no longer needed."

This is presumably something to do with the COM stuff going on behind
the scenes.

Can one of you confirm that if my code looks like this:

try
{
using(DirectoryEntry objADAM = new DirectoryEntry(ldap, admin,
pass, AuthenticationTypes.Secure)
{
using(DirectorySearcher objSearchADAM = new
DirectorySearcher(objADAM)
{
using(SearchResultCollection objSearchResults =
objSearchADAM.FindAll())
{
//Do something with the search results
}
}
}
}
catch(Exception ex)
{
//Handle the error
}

That if my AD code hits a problem inside the try{}, all my directory
objects will be disposed? This is the correct pattern for implementing
Using{} in this scenario?

Many thanks again for your help

Richard

 

[Jon Skeet [C# MVP]]

What you ought to do instead is convert try / finally { Dispose(); } into
using blocks, which automatically handle the null case.

They also cope with the possibility of Dispose calls throwing
exceptions - with a single finally block, if an early Dispose call
fails, the rest won't get executed.

 

[Peter Duniho]

Can one of you confirm that if my code looks like this:

try
{
using(DirectoryEntry objADAM = new DirectoryEntry(ldap, admin,
pass, AuthenticationTypes.Secure)
{
using(DirectorySearcher objSearchADAM = new
DirectorySearcher(objADAM)
{
using(SearchResultCollection objSearchResults =
objSearchADAM.FindAll())
{
//Do something with the search results
}
}
}
}
catch(Exception ex)
{
//Handle the error
}

That if my AD code hits a problem inside the try{}, all my directory
objects will be disposed? This is the correct pattern for implementing
Using{} in this scenario?

Yes, that will work fine (not counting the missing parens, which the
compiler would tell you about ). Though you can clean up the
formatting a bit by putting braces only around the last "using" block and
not indenting the second or third "using":

using(DirectoryEntry objADAM = new DirectoryEntry(ldap, admin, pass,
AuthenticationTypes.Secure))
using(DirectorySearcher objSearchADAM = new
DirectorySearcher(objADAM))
using(SearchResultCollection objSearchResults =
objSearchADAM.FindAll())
{
//Do something with the search results
}

It violates the usual indentation conventions, but is a common variation
and IMHO looks nicer.

Pete

 

[richard.markiewicz]

They also cope with the possibility of Dispose calls throwing
exceptions - with a single finally block, if an early Dispose call
fails, the rest won't get executed.

Thank you Jon.

So what you are saying is, if my original snippet looked like this:

finally
{
objSearchResults.Dispose(); //The troublesome line is now first in the
finally block
objSearchADAM.Dispose();
objADAM.Dispose();
}

That the second and third dispose statements would not be executed
after the first one threw an exception?

Do you know what Using{} does behind the scenes? Does it create a
try{} finally{} on the objects I "use"?

We had a lot of problems with System.DirectoryServices in .NET 1.1
where objects weren't disposed properly, and I'm keen not to go down
that route again. So it would be good to know that my revised pattern
is not subject to the same problem.

Many thanks, Richard

 

[Peter Duniho]

So what you are saying is, if my original snippet looked like this:

finally
{
objSearchResults.Dispose(); //The troublesome line is now first in the
finally block
objSearchADAM.Dispose();
objADAM.Dispose();
}

That the second and third dispose statements would not be executed
after the first one threw an exception?

Yes. That said, you should of course not have your own code have bugs
that throw exceptions, and IMHO the Dispose() method should never throw an
exception. But the "using" statement provides the "belts and suspenders"
to ensure that even if those rules are violated, things get cleaned up.

Do you know what Using{} does behind the scenes? Does it create a
try{} finally{} on the objects I "use"?

Yes. The "using" statement is essentially equivalent to:

IDisposable obj = // whatever your variable is;

try
{
// code in the using block
}
finally
{
obj.Dispose();
}

Nested "using" statements nest the whole thing:

IDisposable obj1 = ...;

try
{
IDisposeable obj2 = ...;

try
{
// code in the using block
}
finally
{
obj2.Dispose();
}
}
finally
{
obj1.Dispose();
}

Thus even if an inner call to Dispose() throws an exception, the outer
ones still get executed.

Note also that the "using" statement essentially generates a private
variable that it uses for the call to Dispose(). It doesn't matter what
you do to the actual declared variable in your own code, the "using"
statement will always call Dispose() on the original reference.

Pete

 

[richard.markiewicz]

Yes. That said, you should of course not have your own code have bugs
that throw exceptions, and IMHO the Dispose() method should never throw an
exception. But the "using" statement provides the "belts and suspenders"
to ensure that even if those rules are violated, things get cleaned up.


Yes. The "using" statement is essentially equivalent to:

IDisposable obj = // whatever your variable is;

try
{
// code in the using block
}
finally
{
obj.Dispose();
}

Nested "using" statements nest the whole thing:

IDisposable obj1 = ...;

try
{
IDisposeable obj2 = ...;

try
{
// code in the using block
}
finally
{
obj2.Dispose();
}
}
finally
{
obj1.Dispose();
}

Thus even if an inner call to Dispose() throws an exception, the outer
ones still get executed.

Note also that the "using" statement essentially generates a private
variable that it uses for the call to Dispose(). It doesn't matter what
you do to the actual declared variable in your own code, the "using"
statement will always call Dispose() on the original reference.

Pete

Thank you Pete for taking the time to explain so clearly.

All the best, Richard

 

[Jon Skeet [C# MVP]]

Yes. The "using" statement is essentially equivalent to:

IDisposable obj = // whatever your variable is;

try
{
// code in the using block
}
finally
{
obj.Dispose();
}

One extra benefit - the finally block is actually:

finally
{
if (obj != null)
{
obj.Dispose();
}
}

In other words, it doesn't matter if the expression you use for the
using statement returns null.

 

[richard.markiewicz]

One extra benefit - the finally block is actually:

finally
{
if (obj != null)
{
obj.Dispose();
}

}

In other words, it doesn't matter if the expression you use for the
using statement returns null.

Great stuff. Thank you Jon.

All the best

Richard