Interesting SPSite leak pattern

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:

 

http://blogs.msdn.com/rogerla/archive/2008/02/12/sharepoint-2007-and-wss-3-0-dispose-patterns-by-example.aspx

12 Comments


  1. 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&gt;

    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!

    Reply

  2. 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

    Reply

  3. Correctly disposing of SharePoint objects like SPWeb and SPSite have been discussed and documented several

    Reply

  4. 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

    Reply

  5. Hi Jonathan,

    yes, you are right. Corrected now.

    Teaches me to let VS.NET sanity check my samples before posting. 😉

    Cheers,

    Stefan

    Reply

  6. Is there a reason not to assign Min and Max the values 0 and spSites.Count respectively? Just iterate over everything?

    Reply

  7. 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

    Reply

  8. 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.

    Reply

  9. 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

    Reply

  10. Recently I came across another very interesting coding pattern which is very similar to the interesting

    Reply

  11. InterestingSPSiteleakpattern

    TodayIcameacrossaveryinterestingcodingpattern:

    publicvoid…

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.