SPWeb.Site, should you call Dispose() on it?

Question

Updated 06/08/2009 15:52: Short answer NO. Original question:

I can't find any reference which gives guidance on SPWeb.Site regarding disposing. I've gone through some of the more popular best practises documentation on disposing SharePoint objects:

• http://www.sharepointdevwiki.com/display/public/When+to+Dispose+SharePoint+objects
• http://msdn.microsoft.com/en-us/library/aa973248.aspx
• http://blogs.msdn.com/rogerla/archive/2008/02/12/sharepoint-2007-and-wss-3-0-dispose-patterns-by-example.aspx

Unfortunately none of these guidelines mention SPWeb.Site. To give some context, I'm writing a public extension API which accepts an SPWeb as an argument to a method i.e.:

public static void GetWebPartFromCatalog(this SPWeb web, string webPartName)
{
     ......

     SPSite site = web.Site;
     ......

     **OR** ??

     using (SPSite site = web.Site)
     {
         ....
     }
}

I've looked as the Close() method in refelector for SPWeb, which is called by SPWeb.Dispose() and there is nothing in it to indicate the actual SPSite member field is disposed of.

Update: 06/08/2009 13:47

At Alex's suggestion

"Put it in a loop which runs 100 times and use the SPRequestStackTrace registry key described in Troubleshooting SPSite/SPWeb leaks in WSS v3 and MOSS 2007 to check that your test code is the source of the problem."

I ran the following piece of code included inside a webpart:

 for (int i = 0; i < 100; i++)
 {
     using (SPWeb web = SPContext.Current.Site.OpenWeb(""))
     {
            SPSite site = web.Site;
            Debug.WriteLine(site.Url);
     }
 }

Nothing appeared in the SharePoint logs.

Whilst I would hesitate to make any real conclusions from this naive experiment, It would suggest that it's not necessary to dispose of SPWeb.Site. It would be really nice to get a concrete answer from someone more informed on this subject.

Update: 06/08/2009 14:52 Prompted by Greg's Comment I worked out the assignments of m_Site and it appears it's ultimately always passed into SPWeb via the internal constructors. E.g. SPWeb.OpenWeb passes in this to new SPWeb(). So I'm more sure that SPWeb.Site should not be disposed, indeed could cause problems if it was.

Hospitality

Looking into this, it's not clear. I asked the same question as a comment to Roger Lamb's post but had no reply. – Explicative

In my opinion, and it's just that, an opinion, no, you should never have to dispose of that object. Why? Well, it's returned through a property from another object. If that other object is disposable, you should dispose that, and let it take care of its own resources. Nothing you read from a property should be disposed by your own code, that would, to me, constitute a bug in that framework. – Trompe

@lasse-v-karlsen I agree with you 100%, however there are many quirks in the SharePoint API and you really need to understand those or risk leaking memory. So whilst it may be a bug in the API I would like to understand if it's there or not :) – Hospitality

Keith Dahlby's journey into trying to work this out for SPList.ParentWeb: solutionizing.net/2008/08/10/is-splistparentweb-a-leak – Explicative

Good discussion and experimentation here - sorry I missed it! :) – Reddin

@Lasse-V-Karlsen I believe the only property that returns an SPSite you should need to dispose is Microsoft.Office.Server.UserProfiles.UserProfile.PersonalSite. – Reddin

Answer 1

Kirk's answer is correct. You must have some handle to an SPSite before an SPWeb can be created, and that is the same SPSite instance you will have when you call SPWeb.Site.

Let's think through the implications of that - if you do not control the creation of the SPSite, but one of its child webs is handed to you from external code, and you dispose the site, when control is returned to the calling code, you've disposed a Site they may not be done with! Put yourself in the calling code's shoes: you pass an SPWeb into a method, and when that method is done, the SPSite you were using has been closed. It is always the instanciator's responsibility to clean up resources they allocate. Do not dispose the SPSite in this case.

Ambit

By the same theory SPLimitedWebPartManager should clean up as well but it doesn't! blogs.msdn.com/rogerla/archive/2008/02/12/… Is there a reason for this or another inconsistency in the API? – Explicative

@Alex I don't see how that is the same. In the example you linked to, the SPSite and SPWeb both still need to be explicitly cleaned up, as well as the other web which is instantiated by the SPLimitedWebPartManager. – Ambit

Very well put Rex. You should always think of an SPWeb as "belonging" to an SPSite, never the other way around. Especially because prematurely disposing an SPSite will first call Dispose() on all SPWebs created from it. As such, I always* assume an SPSite will be disposed by the code that created it. (* The wiki article documents all known cases where SPSite cleanup is your job.) – Reddin

Think I get it now. If I create SPLimitedWebPartManager then it is my job to clean it up and its associated Web property. What I don't understand is the API wasn't designed to do that for me. Another question I guess... – Explicative

I doubt LWPM leaks an SPWeb by design. It was probably just overlooked and will never be fixed because that might break code that cleans up the leak. – Reddin

Answer 2

Just thinking off the top of my head (dangerous sometimes)...

It seems that you cannot really have a SPWeb without having a SPSite. So, if you got the SPWeb without going through SPSite to get it (either by doing a new SPSite or one being provided to you), then you probably do not need to worry about disposing the SPSite.

This is just conjecture, though. Good question!

Rosewater

Thing is since this is an extension method for SPWeb, the caller may well have created the SPSite object. However I don't have a reference to the SPSite object, hence the need to use SPWeb.Site. Which lead to the question , do I need to clean up after myself :) I could ask the caller to include SPSite (add it to the params list) but I didn't want to increase the params on the method. – Hospitality

Answer 3

This isn't clear. There is Stefan's blog that states "You need to ensure that you only dispose SPSite and SPWeb objects that your code owns". Then there is this thread from Michael Washam (Microsoft) that states this pattern does leak.

Unless you can find another reference or someone else knows, why don't you test it out in your development server and add your results as an answer to this question? Put it in a loop which runs 100 times and use the SPRequestStackTrace registry key described in Troubleshooting SPSite/SPWeb leaks in WSS v3 and MOSS 2007 to check that your test code is the source of the problem.

Explicative

Good suggestion, I'll give that a whirl and post the results. – Hospitality

The MSDN thread referenced uses the antipattern of returning an SPWeb reference from a helper method, committing the additional (and more harmful) sin of newing up an SPSite in the process. Instead, that code should be refactored into methods that consume an SPSite/SPWeb argument: solutionizing.net/2009/01/06/elegant-spsite-elevation – Reddin

Answer 4

Reflector tells us that this is the code that runs inside the SPWeb object when you call the Site property:

public SPSite Site
{
    get
    {
        return this.m_Site;
    }
}

It's not creating a new SPSite object, just returning the one it already had, which would be up to the SPWeb.Dispose() to take care of, if necessary. So, you can safely use it, and I would avoid disposing of it, lest SPWeb dependencies go all wonky on you.

Lottie

I guess that really depends on how m_Site is assigned. Using reflector analysis to show "assigned by" on m_Site shows it's assigned in the private member SPWebConstructor() which is passed in site via SPWebs list of internal constructors.... So in the end SPWeb never news up a SPSite. – Hospitality

Answer 5

Have you tried to check your assembly with SPDisposeCheck ? Perhaps it gives you a tip how to handle your problem.

Jacklighter

SPDisposeCheck only verifies those errors on the 'Best Practices' page I believe. – Explicative

Answer 6

I often use this pattern in my SharePoint code as a rule of thumb if calling depose does not crash anything I call it. The other rule that I follow is I try not to create extensions that don't cause a net gain in SPRequest objects (the SPRequest object is the .net object that talks to all of the heavy weight com objcets)

now breaking down your example

 for (int i = 0; i < 100; i++)
 {
     using (SPWeb web = SPContext.Current.Site.OpenWeb(""))
     {
            SPSite site = web.Site;
            Debug.WriteLine(site.Url);
     }
 }

The key here is the SPContext.Current.Site The SPContext will clean up after it's self correctly (one of the few objects that does) since we know that the site is cleaned up correctly I expect that the webs got cleaned up to, however this is far from the correct answer to you question. (note you should be leaking the webs you got via OpenWeb)

public static void GetWebPartFromCatalog(this SPWeb web, string webPartName)

you need to get the SPSite to do thing with the web parts.

1. the web.Site property does in fact clean up after it's self IF the web is disposed.
2. Why not just pass in the SPSite object and let the user of the function worry about it? In most cases I would think that they would be calling using SPContext.Current.Site anyway.
3. Most of the time I have to worry about permissions, not everyone of our dlls end up in the GAC so when I write a new extension method I end up having to wrapper it a SPSecurity.CodeToRunElevated

With those in mind I would end up writing this as . . . now Dispose check will go off in this case because the the SPSite being passed in as an argument because it can't track down in what scope it is disposed in.

public static void GetWebPartFromCatalog(this SPSite site, string webPartName) {
     SPSecurity.CodeToRunElevated( () => {
         using(SPSite suSite = new SPSite(site.Id)){
             //do what you need to do
         }
     };
}

Far

I think you mean SPSecurity.RunWithElevatedPrivileges - CodeToRunElevated is the delegate type. That said, elevating by default in a helper method strikes me as a bad idea. And if you do need to elevate, you're probably better off using impersonation: solutionizing.net/2009/01/06/elegant-spsite-elevation – Reddin

Answer 7

Stefan Gobner has an excellent article on this: Troubleshooting SPSite/SPWeb leaks in WSS v3 and MOSS 2007

...And be sure to use the SPDispose check tool on your codebase.

Janiuszck

Answer 8

You need to delete the SPSite / SPWeb instance if it is a new object

eg

using(SPSite site = new SPSite("url"))
{
    DoSomething;
}

here using will take care of deleting the object however if you have got the reference of the object from the SPContext.Current you should not explicit dispose the object as the reference should be available from SPContext.Current.

Sounding