DeviantPoint

A few weeks ago, I was doing some code reviews when I ran into the following snippet of code:

Example A

SPWeb web = new SPSite(siteGuid).RootWeb;

I told the developer who wrote the code to dispose of these objects he created per the suggestions in the Best Practices: Using Disposable Windows SharePoint Services Objects article. The next morning, the developer sent me an email pinpointing a piece of code he thought I wrote that to him seemed to be the same type of code as his where the objects weren't being destroyed. This was the piece of code:

Example B

SPWeb web = SPContext.Current.Site.RootWeb;

So what was the deal here? Was this a case of "do as I say, not as I do"? No, it wasn't. First of all, the code he was citing wasn't actually written by me, it had been written by another developer. I was, however, the reviewer. Second, I was concerned not only with the web object in his code but the SPSite object he "news up". Both of those objects need to be destroyed which he wasn't doing. At the very least, Example A code should have been rewritten as:

SPSite site = new SPSite(siteGuid);
SPWeb web = site.RootWeb;
//do stuff
web.Dispose();
site.Dispose();

One thing you'll notice here is that I keep a reference to the SPSite object that is being created. That is because this object will later need to be disposed of and by not keeping a reference to this object, there is no way we would be able to do that.

Now back to the RootWeb object, which this developer was pointing out to me also wasn't being disposed of in Example B code. So why not? Is this bad practice not to dispose of the RootWeb object? If you read the best practices article, it clearly states that the SPSite.RootWeb property creates a new object and assigns this new instance to a local member variable. Each subsequent access will be referencing the object stored in the local member variable. Basically, the RootWeb object is lazy loaded on first access. The article later warns that after you're done using the RootWeb property, you should dispose of it. Clearly, Example B doesn't do that. However, not everything is as straightforward as this article suggests.

In the example code in the article, the RootWeb object being accessed is from an SPSite object that was created explicitly. In Example B above, the RootWeb object is being accessed from the SPContext.Current.Site object. As most of you know, the Site and Web properties from SPContext.Current are special in that the disposal of these objects are managed by SharePoint and should not be disposed of explicitly. So what happens then if you dispose of RootWeb from SPContext.Current.Site? Will you get the dreaded "Trying to use an SPWeb object that has been closed or disposed and is no longer valid" error message you might get if you disposed of the Site or Web objects? No you wouldn't. That's because the getter method of the RootWeb property checks to see if the local member variable web object is null. If it is null, then the root web object will just be created and it will be assigned to a local member variable (the lazy loading described above). When the RootWeb is disposed, the reference is set back to null. So any subsequent calls to the property after RootWeb has been disposed of will just reconstruct the object again. So you could dispose of it if you want. But for performance reasons, my suggestion is you shouldn't.

The particular line of code in Example B is actually inside a method in a class that was created to handle a custom localization solution. Potentially, this method could be called 20-25 times per page request. If the RootWeb.Dispose() method is called within this method, then that would mean we would be creating and destroying the same object 20-25 times per page request. That will affect performance. But if I'm suggesting that you don't explicitly dispose of this object you should be asking yourself if we are then creating a memory leak. The answer is no.

When an SPWeb object is created, a reference to this object is added to an internal list of SPWeb objects maintained in the SPSite object to which the SPWeb object belongs. When the SPSite object is disposed, all SPWeb objects in this internal list (m_openedWebs) are iterated over and have their Dispose() methods called also. Since a reference to the root web is added to this internal list of the SPContext.Current.Site object, even though we don't call the SPContext.Current.Site.RootWeb.Dispose() method explicitly, the RootWeb object will eventually be disposed when SharePoint has finished the request and disposes SPContext.Current.Site for us. By following this suggestion, RootWeb will be created and destroyed only once per page request rather than the 20-25 times it would have been had we called RootWeb.Dispose() explicitly.

If you're interested in developing on the SharePoint platform, these are some of the considerations you need to be aware of. As I mentioned in my previous blog entry, in order to really write good SharePoint code, it is important to know the SharePoint object model's implementation. Otherwise, it is very easy to write inefficient code even while we believe we're following all the guidelines and the best practices.

UPDATE: It looks like the Best Practice document has been updated. It is now NOT RECOMMENDED for you to dispose the RootWeb directly. Looks like they've acknowledged that this is being handled internally, as I mentioned above. This is the text from the MSDN article: 

SPSite.RootWeb Property

An earlier version of this article indicated that the calling application should dispose of the SPSite.RootWeb property just before disposing of the SPSite object that is using it. This is no longer the official guidance. The dispose cleanup is handled automatically by the SharePoint framework. Additionally, SPSite properties LockIssue, Owner, and SecondaryContact used the RootWeb property internally. Given the updated guidance for RootWeb, it is no longer advisable to call the Dispose method on the SPSite.RootWeb property whenever any of these properties are used.

I was refactoring some SharePoint code today when I ran into a situation that I think illustrates the challenges with developing in SharePoint. Since object creation and destruction are some of the more expensive operations in SharePoint, I was looking through the code for places where SPSite and SPWeb objects were being unnecessarily created and destroyed. There were a lot of places in the code that followed this typical pattern:

using (SPSite site = new SPSite("http://rootsite")) 

{ 

  using (SPWeb web = site.OpenWeb()) 

  {

    //Do something ....

  }

} 

Now this isn't bad. The site object and the web object are properly wrapped in a using statement so these objects will be properly disposed after we're done using them just like the best practices tells us to do. But in a lot of the places where this type of code was written, it was unnecessary to create brand new site and web objects. We could have just as easily used the SPContext.Current.Site and the SPContext.Current.Web objects. So that's what I started to do. Any code where SPSite and SPWeb objects were being created and destroyed, I refactored to use SPContext.Current.Site and SPContext.Current.Web if it was possible to do so. Again, following best practices, I removed the using statements and made sure not to explicitly call Dispose() on these objects since SharePoint will take care of disposing these objects, unlike the SPSite and SPWeb objects created explicitly.

Every once and a while, I would rebuild, redeploy, and retest the code to make sure nothing was broken. Everything seemed to be going swimmingly. However, after refactoring a particular web part, I started to get the dreaded "Trying to use an SPWeb object that has been closed or disposed ..." error. I checked and re-checked the code. I didn't explicitly call Dispose() on any object in the web part, let alone any SPWeb or SPSite objects. I didn't have any SPWeb or SPSite objects wrapped in a using statement either. So what gave? What was breaking? The code didn't spit out this error before so why was I getting this error now after the refactoring?

I started to go over the places where I changed the code when this particular snippet in the code caught my eye:

using (FullTextSqlQuery sqlQuery = new FullTextSqlQuery(SPContext.Current.Site))

{

    //Do something ...
}

So using Reflector, I disassembled the FullTextSqlQuery class to see what the heck was going on. What I found was that when you construct a new FullTextSqlQuery object (and other objects that derive from the Query object), the SPSite object passed into the constructor is saved to an instance variable (m_site). And when you dispose of your FullTextSqlQuery object instance, it's Dispose() method calls m_site.Dispose(). Since I passed in the SPContext.Current.Site object, when some other code in the request pipeline needed to use this Site object, an error occurred because it had already been disposed.

So the lesson is if you're using the classes derived from the Query class (from either the Microsoft.SharePoint.Search.Query or the Microsoft.Office.Server.Search.Query namespaces) and you use the constructor that accepts an SPSite object, then be careful not to pass an SPSite instance that is needed after the Query object is destroyed. That means never pass it the SPContext.Current.Site object nor any SPSite object you construct yourself that you'll need after the Query object's lifetime.

Now back to my original point: This is simply another example of what can make SharePoint development difficult. When using the SharePoint object model, it is often necessary to know the implementation of the SharePoint object you're trying to use otherwise you may not know how to handle the object properly (as in the case of disposing SPWeb and SPSite objects) or may not know of the side effects introduced by the object. In this case, I had to crack open the code for the FullTextSqlQuery class in order for me to see what side effects occurred when using this class. And this is not the first time I've had to do that (I'll post about another case in a future blog).

I think that SharePoint is a fantastic product and definitely a viable development platform. In my opinion, the benefits and features of SharePoint far outweigh the challenges the product can sometimes present (from a development perspective). But API/Framework developers should strive to avoid forcing their consumers to have to know the inner workings of their framework in order to use it correctly and in this regard, I think there is still some room for improvement in SharePoint.