Sharepoint SPDisposeCheck

Question

The SPDisposeCheck utility alerted me to an impproperly disposed SPWeb.Add call. As you can see below, the typical using(SPWeb NewWeb = webs.add(siteUrl ....) method would not work because of the RunWithElevatedPrivileges would make the return newWeb out of context.

By looking at the newWeb = webs.Add() line below, can anyone suggest a way to properly dispose the new SPWeb object? Thanks in advance.

public partial class SiteHelper          
{          
    public static SPWeb CreateSiteFromSTP(SPWeb parentWeb, string newSiteSTP, int teamId)   
    {          
     try       
     {        
      SPWeb newWeb = null;    
      SPSecurity.RunWithElevatedPrivileges(delegate()    
      {       
       string siteUrl = teamId.ToString();    
       SPWebCollection webs = parentWeb.Webs;   
       newWeb = webs.Add(siteUrl,.,.,.,);
       TraceProvider.WriteLine("Activating Feature : MembersFeature ");    
       newWeb.Features.Add(new Guid(TeamSiteAttributes.MembersFeature), true);   
       TraceProvider.WriteLine("Activating Feature : BadgeAwardsFeature ");    
       newWeb.Features.Add(new Guid(TeamSiteAttributes.BadgeAwardsFeature), true);   
       TraceProvider.WriteLine("Activating Feature : ProjectBenefitsFeature ");   
       newWeb.Features.Add(new Guid(TeamSiteAttributes.ProjectBenefitsFeature), true);   
       TraceProvider.WriteLine("Activating Feature : TeamScoreFeature ");    
       newWeb.Features.Add(new Guid(TeamSiteAttributes.TeamScoreFeature), true);   
       newWeb.Update();     
       parentWeb.Update();     
      });       
      return newWeb;     
     }   
     catch (Exception ex)    
     {        
      TraceProvider.WriteLine("Error", ex);     
      throw;
     }
    }   
}

Answer 1

SPDisposeCheck is reporting this as an error because it's not smart enough to know what you are doing with newWeb once you've returned it from this method. As long as you dispose of newWeb after your call to CreateSiteFromSTP() then you won't have a memory leak.

If you are confident that you don't have a memory leak in this method, you can set SPDisposeCheck to ignore just this particular warning. Just add the following declaration (with the correct SPDisposeCheckID number you received) above your CreateSiteFromSTP method:

[SPDisposeCheckIgnore(SPDisposeCheckID.SPDisposeCheckID_110, "Caller will dispose")]

Answer 2

This is mostly a question of best practices for safe cleanup in this case... would it be better for this method to return a GUID or the url for the new SPWeb rather than the actual SPWeb that was created? That way this method could properly dispose of the SPWeb it created, and the caller would still have an opportunity to easily create another SPWeb whose lifetime is less mysterious. What's the real cost in creating an SPWeb compared to the risk of passing one around and potentially neglecting proper cleanup?

Answer 3

The proper way to dispose of your new SPWeb is in the scope where it was created. If you need to perform additional operations on your new web, just pass in a delegate to call:

public static void CreateSiteFromSTP(SPWeb parentWeb, string newSiteSTP, int teamId, Action<SPWeb> actionOnCreate)       
{                                                           
    // ...
    using(var newWeb = webs.Add(...))
    {
        // ...
        newWeb.Update();
        actionOnCreate(newWeb);
    }
}

Then you can just pass in a method (anonymous or named) that manipulates your new SPWeb, without responsibility for disposal being passed around. That approach also has the benefit of not requiring that you return the SPWeb outside your elevated block, which is unsupported and unreliable.

In fact, I would be surprised to find that your code actually works as you intend: existing SharePoint objects (specifically parentWeb) have their permissions set when they are created and should also not be passed into an elevated context. A better approach for elevating permissions within SharePoint is to use SPSite impersonation. Using a RunAsSystem method defined here, I would refactor your code like this:

public static void ElevateToCreateSiteFromSTP(SPWeb parentWeb, string newSiteSTP, int teamId, Action<SPWeb> actionOnCreate)
{
    parentWeb.RunAsSystem(elevWeb =>
        CreateSiteFromSTP(elevWeb, newSiteSTP, teamId, actionOnCreate));
}

private static void CreateSiteFromSTP(SPWeb parentWeb, string newSiteSTP, int teamId, Action<SPWeb> actionOnCreate)
{                                                           
    try                                     
    {
        string siteUrl = teamId.ToString();             
        SPWebCollection webs = parentWeb.Webs;          
        using(var newWeb = webs.Add(siteUrl, ...))
        {
            var newWebFeatures = newWeb.Features;

            TraceProvider.WriteLine("Activating Feature : MembersFeature ");           
            newWebFeatures.Add(new Guid(TeamSiteAttributes.MembersFeature), true);         
            TraceProvider.WriteLine("Activating Feature : BadgeAwardsFeature ");            
            newWebFeatures.Add(new Guid(TeamSiteAttributes.BadgeAwardsFeature), true);     
            TraceProvider.WriteLine("Activating Feature : ProjectBenefitsFeature ");        
            newWebFeatures.Add(new Guid(TeamSiteAttributes.ProjectBenefitsFeature), true);
            TraceProvider.WriteLine("Activating Feature : TeamScoreFeature ");              
            newWebFeatures.Add(new Guid(TeamSiteAttributes.TeamScoreFeature), true);       

            newWeb.Update();                        
            parentWeb.Update();                     

            if(actionOnCreate != null)
                actionOnCreate(newWeb);
        }
    }       
    catch (Exception ex)
    {                                               
            TraceProvider.WriteLine("Error", ex);                   
            throw;
    }
}

This has the added benefit of separating your elevation concerns from the logic to create the SPWeb. I also prefer to make it quite obvious where my code will be running with different permissions.