 |
 | Home | Contact Us | FAQ | Search & Site Map | Link to Us |
 | Sign In | Join | Other 45 Sites in Network |
.NET Forum / Languages / C# / January 2008Is this pattern OK?
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 > [...] > 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 > Hi Gurus, > [quoted text clipped - 50 lines] > 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. > The thing I can't work out is why this only started happening in the > last week! > > Many thanks for any advice, Richard > <richard.markiew...@eshareuk.com> wrote in message > [quoted text clipped - 62 lines] > > > Many thanks for any advice, Richard 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 > Can one of you confirm that if my code looks like this: > [quoted text clipped - 22 lines] > 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 <snip> > 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.  SignatureJon Skeet - <skeet@pobox.com> http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet World class .NET training in the UK: http://iterativetraining.co.uk > <snip> > [quoted text clipped - 8 lines] > Jon Skeet - <sk...@pobox.com>http://www.pobox.com/~skeet Blog:http://www.msmvps.com/jon.skeet > World class .NET training in the UK:http://iterativetraining.co.uk 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 > So what you are saying is, if my original snippet looked like this: > [quoted text clipped - 8 lines] > 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 On Jan 18, 3:08 pm, "Peter Duniho" <NpOeStPe...@nnowslpianmk.com> wrote: > On Fri, 18 Jan 2008 11:50:43 -0800, richard.markiew...@eshareuk.com > [quoted text clipped - 64 lines] > > Pete Thank you Pete for taking the time to explain so clearly. All the best, Richard <snip> > Yes. The "using" statement is essentially equivalent to: > [quoted text clipped - 8 lines] > 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. SignatureJon Skeet - <skeet@pobox.com> http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet World class .NET training in the UK: http://iterativetraining.co.uk > <snip> > [quoted text clipped - 28 lines] > Jon Skeet - <sk...@pobox.com>http://www.pobox.com/~skeet Blog:http://www.msmvps.com/jon.skeet > World class .NET training in the UK:http://iterativetraining.co.uk Great stuff. Thank you Jon. All the best Richard Free MagazinesGet these publications absolutely FREE for up to 12 months. There are no hidden fees and no obligation. Simply choose a title, complete the application form and submit it. Read more ...
|
Reply to this Message
|
 Sign In
 Join
 My Latest Posts My Monitored Threads My Blog My Photo Gallery My Profile My Homepage Start New Thread Enable EMail Alerts Rate this Thread
|
©2008 Advenet LLC Privacy Policy - Terms of Use
This website includes both content owned or controlled by Advenet as well as content owned or controlled by third parties.