Today I came across a very interesting coding pattern:
public void enumSiteCollection(SPSiteCollection spSites, int min, int max)
{
for (int i = min; i<max; i++)
{
if (spSites[i] != null)
{
DoSomething(spSites[i]);
spSites[i].Dispose();
}
}
}
On a first look the code look ok, right? Ok, no try/catch blog and so on but for normal operations it looks as if the SPSite objects used from the SPSiteCollection object are correctly disposed, right?
The problem here is hidden in the internal implementation of the indexer of the SPSiteCollection object. What happens under the hood is that for every single use of spSites[i] a new independent SPSite object is created.
That means the code above creates 3 SPSite objects – but only disposes one.
A correct implementation of the code would look like this:
public void enumSiteCollection(SPSiteCollection spSites, int min, int max)
{
for (int i = min; i<max; i++)
{
SPSite site = spSites[i];
if (site != null)
{
DoSomething(site);
site.Dispose();
}
}
}
That will ensure that only one single SPSite object is created, reused and finally disposed.
Ok, completely correct it should be something like this:
public void enumSiteCollection(SPSiteCollection spSites, int min, int max)
{
SPSite site = null;
for (int i = min; i<max; i++)
{
try
{
site = spSites[i];
if (site != null)
{
DoSomething(site);
}
}
finally
{
if (site != null)
{
site.Dispose();
}
}
}
}
or this:
public void enumSiteCollection(SPSiteCollection spSites, int min, int max)
{
for (int i = min; i<max; i++)
{
using (SPSite site = spSites[i])
{
if (site != null)
{
DoSomething(site);
}
}
}
}
Here you can find more coding patterns which need to be avoided:
Permalink
Stefan, it amazes me how many of these you keep finding!
Myself and Keith are trying to keep the SharePointDevWiki.com page up to date with everyones findings and having references to their posts e.g. yourself, Roger, Keith, Scott Harris etc.
Reading through the lines on this one…is this covered by this area of the wiki page we’ve created?
<a href="http://www.sharepointdevwiki.com/display/public/When+to+Dispose+SharePoint+objects#WhentoDisposeSharePointobjects-SPWebCollection">http://www.sharepointdevwiki.com/display/public/When+to+Dispose+SharePoint+objects#WhentoDisposeSharePointobjects-SPWebCollection</a>
In the example on the wiki page there is a using statement rather than a try catch finally block.
Please feel free to contribute to this page. People find it easier to see all this in one spot rather than having to bookmark multiple posts.
Thanks again for sharing this with the community!
Permalink
PingBack from http://stevepietrek.com/2009/01/29/links-1292009/
Permalink
Hi Jeremy,
from what I can see the wiki does not cover it yet.
Regarding contribution to the wiki: I’m maintaining my blog – that takes already lots of time.
I cannot spend additional time to update external sites as well.
Feel free to update the wiki and link to my article if you like.
Cheers,
Stefan
Permalink
Correctly disposing of SharePoint objects like SPWeb and SPSite have been discussed and documented several
Permalink
Hi
Thanks for the tip. I’d better not forget it now.
Just one tiny thing however, correct me if I’m wrong (I don’t have any Visual Studio available for the test right now) but shouldn’t the SPSite object be declared before the Try{} block to be available in the Finally{} block scope ?
Thanks,
Jonathan
Permalink
Hi Jonathan,
yes, you are right. Corrected now.
Teaches me to let VS.NET sanity check my samples before posting. 😉
Cheers,
Stefan
Permalink
Is there a reason not to assign Min and Max the values 0 and spSites.Count respectively? Just iterate over everything?
Permalink
Hi Andy,
the logic here does not require all SPSite objects.
That’s why a for loop and no foreach loop is used.
Cheers,
Stefan
Permalink
Do you maybe have any idea why the indexer of SPSiteCollection was implemented that way?
When working with a collection of objects, I came to expect that I would work with references, but it seems the developers have embraced the idea of passing objects by value.
Permalink
Hi Helmut,
internally the SPSiteCollection is actually nothing but a DataTable with a row for each SPSite and columns for the different properties.
When accessing an indivial SPSite through an index the SPSite object has to be constructed from the DataTable.
Cheers,
Stefan
Permalink
Recently I came across another very interesting coding pattern which is very similar to the interesting
Permalink
InterestingSPSiteleakpattern
TodayIcameacrossaveryinterestingcodingpattern:
publicvoid…