Introducing SPWeb.GetParentWeb()

Official Microsoft guidance is to never explicitly Dispose() SPWeb.ParentWeb. I generally agree with this advice, given that my Rule #1 of SharePoint disposal is that “Using a disposed object can cause more problems than failing to dispose.” To understand why, I’ll borrow my explanation from SPDevWiki:

This property will allocate an SPWeb object the first time it is called. The caveat is that once it is disposed, any reference to the property will return the disposed object. If an SPWeb is not owned by the developer, its ParentWeb should be considered not owned as well. For example, there could be a problem if two components both depend on SPContext.Current.Web.ParentWeb and one calls Dispose() before the other is done with it.

However, this can result in memory pressure in cases involving enumeration or where the parent SPSite has a long lifetime. For example:

SPSite contextSite = SPContext.Current.Site;
foreach(SPWeb web in contextSite.AllWebs.AsSafeEnumerable())
{
    SPWeb webParent = web.ParentWeb; // Internal OpenWeb()
    // Do something with web and webParent
}

The web references are disposed by my safe iterator, but every webParent will remain open until the context SPSite is disposed. Not that I would recommend using code like this (in fact I would strongly urge against it), but you can never say never.

To that end, I propose a simple extension method whose contract is clear: always dispose me! We can still follow MS guidance regarding SPWeb.ParentWeb, but have convenient access to a developer-owned parent SPWeb as well:

[SPDisposeCheckIgnore(SPDisposeCheckID.SPDisposeCheckID_120, "By Design")]
public static SPWeb GetParentWeb(this SPWeb web)
{
    if(web == null)
        throw new ArgumentNullException("web");
    return web.Site.OpenWeb(web.ParentWebId);
}

And our “Best Practice” memory pressure can be revised slightly to achieve much better memory use:

SPSite contextSite = SPContext.Current.Site;
foreach(SPWeb web in contextSite.AllWebs.AsSafeEnumerable())
{
    using(SPWeb webParent = web.GetParentWeb())
    {
        // Do something with web and webParent
    }
}

Trivial? Obvious? Perhaps. But often the most useful code is.

Update: Included appropriate SPDisposeCheckIgnore attribute for “leaked” SPWeb from OpenWeb(); we know what we’re doing. That said, you could certainly implement higher-order functions to invoke an action or selector against our imitation ParentWeb without returning it—I’ll leave those as an exercise for the reader.

Advertisements

SPList.ParentWeb Leaks Revisited

A while back I looked into whether or not SPList.ParentWeb really needs to be disposed: Is SPList.ParentWeb a Leak? The specific motivation was to investigate why SPList.BreakRoleInheritance() requires Dispose() on ParentWeb property, as seen in Roger’s Dispose Patterns by Example. I stand by my original conclusion that this advice generally does more harm than good, but thought it would be useful to discuss in a bit more detail.

Why dispose ParentWeb?

As I showed before, the code for SPList.ParentWeb works like this:

if (SPUtility.StsCompareStrings(this.m_Lists.Web.ServerRelativeUrl, this.ParentWebUrl))
  return this.m_Lists.Web;

if (this.m_parentWeb == null)
  this.m_parentWeb = this.m_Lists.Web.Site.OpenWeb(this.ParentWebUrl);
return this.m_parentWeb;

In the vast majority of cases, the URLs will match and return the parent collection’s Web, which should not be disposed (unless, of course, you own it and know it is ready for disposal). Only in the exceptional case that the list’s ParentWebUrl indicates it doesn’t live with its parent collection will a new SPWeb be created. I believe it is this exception, rather than the norm, that leads to Roger’s suggestion that ParentWeb should be disposed.

On a curious and somewhat related note, SPListItem.Web doesn’t use ParentWeb:

public SPWeb get_Web()
{
    return this.m_Items.List.Lists.Web;
}

Why BreakRoleInheritance()?

Though BreakRoleInheritance() has been singled out, the real culprit is an internal property:

private SPSecurableObjectImpl get_SecurableObjectImpl()
{
    if (this.m_SecurableObjectImpl == null)
    {
        Guid guidScopeId = (Guid) this.m_arrListProps[0x22, this.m_iRow];
        bool hasUniquePerm = guidScopeId != this.ParentWeb.RoleAssignments.Id;
        this.m_SecurableObjectImpl = new SPSecurableObjectImpl(this.ParentWeb, this, ...);
    }
    return this.m_SecurableObjectImpl;
}

This property is then used in the implementations of several methods/properties from ISecurableObject:

  • SPList.AllRolesForCurrentUser
  • SPList.BreakRoleInheritance()
  • SPList.HasUniqueRoleAssignments
  • SPList.ResetRoleInheritance()
  • SPList.ReusableAcl
  • SPList.RoleAssignments

(Incidentally, SPSecurableObjectImpl is also used by SPWeb and SPListItem to implement that interface.)

SPList.get_SecurableObjectImplSo now we’re dealing with several more points of entry. For example, in SPListItem:

public ISecurableObject get_FirstUniqueAncestor()
{
    this.InitSecurity();
    if (this.RoleAssignments.Id == this.ParentList.RoleAssignments.Id)
    {
        return this.ParentList.FirstUniqueAncestor;
    }
    if (!this.HasUniqueRoleAssignments)
    {
        return this.Web.GetFolder(this.m_strPermUrl).Item;
    }
    return this;
}

I could show several more obscure code paths with the same result, but the specifics are irrelevant. My point is that trying to figure out if SPList.ParentWeb has been referenced is a wasted effort.

What to do?

The way I see it, we have two options:

  1. Assume that ParentWeb is safe enough and if we have memory problems later we can investigate.
  2. Assume that ParentWeb is leaky and figure out a safe way to Dispose() it.

I’m satisfied with the former, but if you’re not — or if you have a circumstance where the ParentWebUrl comparison actually fails (please share) — then your code should check if it needs to dispose first:

SPWeb web = SPContext.Current.Web;
SPList list;

try
{
  list = web.Lists["ListName"];
  list.BreakRoleInheritance(true);
}
finally
{
  if(list.ParentWeb != web)
    list.ParentWeb.Dispose();
}

Or we can use an extension method:

public static void DisposeParentWeb(this SPList list)
{
  if(list.ParentWeb != list.Lists.Web)
    list.ParentWeb.Dispose();
}

Is SPList.ParentWeb a Leak?

As previously discussed, doing the “right thing” with our SPSite and SPWeb references’ disposability is way harder than it should be. And even worse is the fact that the experts don’t seem to agree how that should be done. Now I’m definitely not an expert, and I’m not even sure who’s moderating the debate, but I’m intrigued so I guess I’ll invite myself anyway.

First, the code in question, from a useful STSADM extension by Gary Lapointe:

  lvw = new ListViewWebPart();
  SPList list = Utilities.GetListFromViewUrl(listUrl);
  if (list == null)
    throw new ArgumentException("List not found.");

  lvw.ListName = list.ID.ToString("B").ToUpperInvariant();
  lvw.TitleUrl = list.DefaultViewUrl;
  lvw.WebId = list.ParentWeb.ID;

The question: Does list.ParentWeb need to be disposed? My first instinct was that it does, based on this quote from Roger Lamb’s dispose patterns:

When using the SPList.BreakRoleInheritance() method a internal call to ParentWeb property is called and must be disposed by the caller.

But upon further consideration, is that even true? Well, let’s reflect SPList.ParentWeb to see what we’re dealing with:

    if (!SPUtility.StsCompareStrings(this.m_Lists.Web.ServerRelativeUrl, this.ParentWebUrl)) {
      if (this.m_parentWeb == null) {
        this.m_parentWeb = this.m_Lists.Web.Site.OpenWeb(this.ParentWebUrl);
      }
    }
    else {
      return this.m_Lists.Web;
    }
    return this.m_parentWeb;

Or rewritten for clarity:

    if (SPUtility.StsCompareStrings(this.m_Lists.Web.ServerRelativeUrl, this.ParentWebUrl))
        return this.m_Lists.Web;

    if (this.m_parentWeb == null)
        this.m_parentWeb = this.m_Lists.Web.Site.OpenWeb(this.ParentWebUrl);
    return this.m_parentWeb;

So if the list’s parent collection matches its ParentWebUrl, it uses the collection’s Web; if not it returns m_parentWeb, which Reflector verifies is only set by the preceding call to Site.OpenWeb(). As far as I can tell, every call to OpenWeb requires a matching call to Dispose, and as SPList isn’t IDisposable that responsibility would seem to fall to the developer. So we should Dispose, right? Well, maybe… We can’t forget about the first if: OpenWeb is only called if the list isn’t in its collection’s web. (Bonus question: how could that happen?)

So it seems the most common scenario is that SPList.ParentWeb will just defer to SPListCollection.Web, which returns SPListCollection.m_web. And m_web is only set by an internal SPListCollection constructor, used by SPWeb.Lists:

    if (this.m_Lists == null)
      this.m_Lists = new SPListCollection(this);
    return this.m_Lists;

So with all that in mind, consider the following code:

    SPWeb web = SPContext.Current.Web;
    SPList list = web.Lists["ListName"];
    list.BreakRoleInheritance();
    list.ParentWeb.Dispose(); // Best practice?

BreakRoleInheritance does indeed have an internal reference to ParentWeb (hidden in SPList.SecurableObjectImpl), but in this case ParentWeb will be web, which is our context web and should not be disposed!

So back to Gary’s example. Before we can decide how to handle list.ParentWeb, we should check out the helper that created list for us:

    internal static SPList GetListFromViewUrl(string url) {
      using (SPSite site = new SPSite(url))
      using (SPWeb web = site.OpenWeb()) {
        return GetListFromViewUrl(web, url);
      }
    }

Since the SPWeb was opened based on the list URL, it should be safe to assume that ParentWeb won’t need to call OpenWeb again and will instead use Lists.Web, which will be a reference to the original webwhich is disposed (by using) in the helper. So list.ParentWeb indeed doesn’t need to be disposed because it’s already been disposed. In practice we can probably get away with it (in this case), but there are no guarantees that a disposed SPWeb will be in a consistent state when we get around to using it.

Though we’ve answered the original question of disposal, we should really eliminate the post-disposal reference. My preference is to refactor the ListViewWebPart code into helpers modeled after Gary’s existing GetListFromViewUrl methods:

    internal static ListViewWebPart GetListViewWebPartFromViewUrl(string url) {
      using (SPSite site = new SPSite(url))
      using (SPWeb web = site.OpenWeb()) {
        return GetListViewWebPartFromViewUrl(web, url);
      }
    }

    internal static ListViewWebPart GetListViewWebPartFromViewUrl(SPWeb web, string url) {
      ListViewWebPart lvw;
      lvw = new ListViewWebPart();
      SPList list = GetListFromViewUrl(web, url);
      if (list == null)
        throw new ArgumentException("List not found.");

      lvw.ListName = list.ID.ToString("B").ToUpperInvariant();
      lvw.TitleUrl = list.DefaultViewUrl;
      lvw.WebId = web.ID;
      return lvw;
    }

Which greatly simplifies the original code and eliminates the ParentWeb risk:

  lvw = Utilities.GetListViewWebPartFromViewUrl(listUrl);

Conclusions

So back again to the original question: Does SPList.ParentWeb need to be disposed? The answer seems to be a qualified “probably not”. But even if we’re not leaking an SPWeb reference, more concerning is the potential for ParentWeb to return an object that has already been disposed. If nothing else, be careful with ParentWeb on lists returned from helpers that might have cleaned up prematurely.

Disposing list’s SPSite/SPWeb without ParentWeb

Update 12/10/2008: Don’t use this technique; use the guidance here instead: The New Definitive SPSite/SPWeb Disposal Article

Chris O’Brien’s recent post discusses the need to dispose of SPSite and SPWeb objects, but only if they didn’t come from SPContext. To do this he depends on a list’s ParentWeb property, which currently returns the same instance as the original SPWeb. However, he notes that this strategy depends on the internal implementation of ParentWeb, which (however unlikely) might change.

As a future-proof alternative, I propose capturing the SPWeb as an out parameter if it will need to be disposed:

public void DoSomething() {
  SPWeb webToDispose = null;
  SPList list = getList(out webToDispose);

  // do something with list..
  foreach (SPListItem item in list.Items) {
    processItem(item);
  }

  // ** PROBLEM - how do we now dispose of the SPSite/SPWeb objects we created earlier? **
  // ** SOLUTION - if we didn't use context, webToDispose has reference
  if (webToDispose != null) {
    webToDispose.Dispose();
    webToDispose.Site.Dispose();
  }
}

private SPList getList(out SPWeb webToDispose) {
  webToDispose = null;

  SPContext currentContext = SPContext.Current;
  SPList list = null;

  if (currentContext != null) {
    list = currentContext.Site.AllWebs["MyWeb"].Lists["MyList"];
  }
  else {
    SPSite site = new SPSite("http://solutionizing.net");
    webToDispose = site.OpenWeb("/MyWeb");
    list = webToDispose.Lists["MyList"];
  }

  return list;
}

This code should be functionally equivalent and perhaps a bit easier to read, but without the dependency on ParentWeb. Thoughts?

Also, to add to Chris’s list of required reading, check out Roger Lamb’s excellent SharePoint 2007 and WSS 3.0 Dispose Patterns by Example.

Update 7/23/2008: How ironic…Chris and I have a leak! If you can’t spot it, I explain here.