More SharePoint Higher-Order Functions

Though I haven’t actually used the term before, I’ve discussed a number of higher-order functions in the past. Simply put, a higher-order function either accepts a function as a parameter, returns a function, or both. The terminology might be foreign, but the technique is used all over the place:

Another use of higher-order functions is to ensure the existence of a SharePoint resource. For example, I often need to fetch a SharePoint list and create it if doesn’t exist. A standard implementation might look something like this:

public static SPList GetOrCreateList(this SPWeb web, string listName,
                                     string description, SPListTemplate template)
{
    SPListCollection webLists = web.Lists;
    SPList list = webLists.Cast<SPList>()
                          .FirstOrDefault(l => l.Title == listName);
    if (list == null)
    {
        Guid newListID = webLists.Add(listName, description, template);
        list = webLists[newListID];
    }
    return list;
}

While there’s nothing wrong with this implementation, per se, it’s not exceedingly flexible. What if we want to use a different overload of SPListCollection.Add? What if we need to elevate privileges to create the list? We could certainly create a dozen variations based on this pattern, but that’s a bunch of duplicate code that we would much rather avoid. Instead, we can use a single higher-order function:

public static SPList GetOrCreateList(this SPWeb web, string listName,
                                     Func<SPWeb, string, SPList> listBuilder)
{
    SPList list = web.Lists.Cast<SPList>()
                     .FirstOrDefault(l => l.Title == listName);
    if(list == null && listBuilder != null)
        list = listBuilder(web, listName);
    return list;
}

And then specify exactly how the list should be created. We could redefine our original method like this:

public static SPList GetOrCreateList(this SPWeb web, string listName,
                                     string description, SPListTemplate template)
{
    return GetOrCreateList(web, listName, (builderWeb, builderName) =>
    {
        var builderLists = builderWeb.Lists;
        Guid newListID = builderLists.Add(builderName, description, template);
        return builderLists[newListID];
    });
}

Or we can just as easily specify a builder that uses elevated privileges and a different Add overload:

public static SPList GetOrCreateTasksList(this SPWeb web)
{
    return GetOrCreateList(web, "Tasks", (builderWeb, builderName) =>
    {
        Guid newListId = web.SelectAsSystem(sysWeb =>
            sysWeb.Lists.Add(builderName, null, SPListTemplateType.Tasks));

        return builderWeb.Lists[newListId];
    });
}

Or my preference is to define a (testable) builder method and just use the higher-order function without a wrapper:

private static SPList CreateGenericList(SPWeb web, string name)
{
    var id = web.Lists.Add(name, null, SPListTemplateType.GenericList);
    return web.Lists[id];
}

void DoSomething(SPWeb web)
{
    SPList list = web.GetOrCreateList("Some List", CreateGenericList);
    if (list == null)
        throw new SPException("Some List does not exist and could not be created.");
    // Do something
}

GetOrCreateGroup

Another use for this pattern is the creation of SharePoint groups, inspired by Adam Buenz’s recent post. His code is correct (though I believe an ordinal comparison is more appropriate than invariant culture), but it can’t easily handle scenarios requiring elevation, AllowUnsafeUpdates, etc. Instead, we can define a higher-order function like this:

public static SPGroup GetOrCreateGroup(this SPWeb web, string groupName,
                                       Func<SPWeb, string, SPGroup> groupBuilder,
                                       Action<SPWeb, SPGroup> associateGroup)
{
    SPGroup group = web.SiteGroups.Cast<SPGroup>()
                        .FirstOrDefault(g =>
                            string.Equals(g.Name, groupName,
                                StringComparison.OrdinalIgnoreCase));
    if (group == null && groupBuilder != null)
        group = groupBuilder(web, groupName);
    if (group != null && associateGroup != null)
        associateGroup(web, group);
    return group;
}

With which the original method is easily rewritten:

public static SPGroup GetGroupOrCreate(SPWeb web, string name,
                                       string description, SPUser owner,
                                       SPUser defaultUser, bool associate)
{
    return web.GetOrCreateGroup(name,
        (builderWeb, builderName) =>
        {
            var builderGroups = builderWeb.SiteGroups;
            builderGroups.Add(builderName, owner, defaultUser, description);
            return builderGroups[name];
        },
        (assocWeb, assocGroup) =>
        {
            if (associate && !assocWeb.AssociatedGroups.Contains(assocGroup))
            {
                web.AssociatedGroups.Add(assocGroup);
                web.Update();
            }
        });
}

Again, the advantage is that we can easily tweak how the group is created and associated independent from the common get-or-create logic.

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.