Sunday 15 June 2008

Disposing SharePoint objects - what they don't tell you

If you're a SharePoint developer and find yourself regularly writing code to work with SPListItem objects, here's some info which might help you factor your code in a slightly neater way in some scenarios.  Interestingly this doesn't get mentioned in any of the key papers/articles - I can think of a good reason for this (which we'll look at), but in my mind this is a safe technique. If you have other ideas however, I'd love to hear from you.

Most developers are now fairly well-versed in the techniques needed to write efficient SharePoint code (key resources are listed at the end of the article), specifically in terms of disposing of any SPSite/SPWeb objects your code creates. The thing is, having to always be mindful of disposing these objects can sometimes restrict us in the way we want to write our code - consider the following simplified example:

public void DoSomething()
{
SPList list = getList();

// 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? **
}

private SPList getList()
{
// can't dispose of these objects here if we're returning a list - we'll be attempting to use
// objects which have already been disposed of..
SPSite site = new SPSite("http://cob.blog.dev");
SPWeb web = site.OpenWeb("/MyWeb");
SPList list = web.Lists["MyList"];

return list;
}


The problem here is we created our SPSite/SPWeb objects in a different method to where we need to dispose them - this can happen a lot when writing library code. To extend this example, here's a pattern I regularly use to ensure I'm getting my objects the most efficient way - using SPContext if it's present, but instantiating them myself if it isn't (i.e. the code is called from an event receiver, console app etc.):


public void DoSomething()
{
bool bDisposalsRequired = false;

// get list from SPContext if we have one..
SPList list = getListFromContext();
if (list == null)
{
// otherwise get list from objects we create..
list = getInstantiatedList();
bDisposalsRequired = true;
}

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

if (bDisposalsRequired)
{
// ** PROBLEM - how do we now dispose of the SPSite/SPWeb objects we created earlier? **
}
}

private SPList getInstantiatedList()
{
// can't dispose of these objects here if we're returning a list - we'll be attempting to use
// objects which have already been disposed of..
SPSite site = new SPSite("http://cob.blog.dev");
SPWeb web = site.OpenWeb("/MyWeb");
SPList list = web.Lists["MyList"];

return list;
}

private SPList getListFromContext()
{
SPContext currentContext = SPContext.Current;
SPList list = null;

if (currentContext != null)
{
list = currentContext.Site.AllWebs["MyWeb"].Lists["MyList"];
}

return list;
}


When the code starts to become more complex like this, we start to really want a simple way of disposing the objects we created elsewhere. I'd probably caution against this, but what happens if you created the SPSite/SPWeb objects not only in another method, but another class? All of a sudden disposing of objects correctly is a lot more complex than the examples given in the white papers. 

Now, the savvy SharePoint developer might think "Hold on, the SPList has a ParentWeb object (and SPListItem has a ParentList object!), what happens if I go up the hierarchy and dispose that way?" - code to this would look like:


if (bDisposalsRequired)
{
list.ParentWeb.Dispose();
list.ParentWeb.Site.Dispose();
}


Now how would we know the correct objects have been disposed? Are these objects the same objects we were using earlier? Or has SharePoint populated these properties with references to different objects, meaning we've left our original objects alive and could run into scalability problems?

Well, the good news is these are the same objects, so disposing in this way will dispose of the objects you created. We can tell this by comparing the objects using System.Object's GetHashCode() method:


private void compareObjects()
{
SPSite site = new SPSite("http://cob.blog.dev");
SPWeb web = site.OpenWeb("/MyWeb");
SPList list = web.Lists["MyList"];

SPWeb parentWeb = list.ParentWeb;



// no message displayed..
Debug.Assert(parentWeb.GetHashCode() != web.GetHashCode(), "Objects are not same!");


// create a *new* SPWeb object and compare it to our other reference..
web = site.OpenWeb("/MyWeb");

// message IS displayed..
Debug.Assert(parentWeb.GetHashCode() != web.GetHashCode(), "Objects are not same!");
}


What we see from this is that the SPWeb object stored in the list.ParentWeb property has the same hash code as the original SPWeb object we created, but if we create a new instance of the same web, it has a different hash code. This indicates the objects in the first comparison are the same - so disposing of them through the ParentWeb reference will indeed dispose of the objects we created. So happy days - we've found we can factor our code in a way which may be more logical and readable.


The caveat

So why might this not be mentioned in any of the published guidance? Well, before using this technique throughout your code you should consider that in writing code in this way we are effectively relying on an internal implementation detail of the current SharePoint API. If Microsoft were to change this in a service pack (or the next version of SharePoint), our code may no longer dispose of objects correctly. I'd recommend giving this your own consideration (not least because you might conclude differently to me), but I'd suggest this change would be fairly unlikely. The reason for this is the SharePoint team themselves need to eliminate any unnecessary SPSite/SPWeb objects, so of course it makes sense that the the same SPWeb instance used to obtain the SPList is used to populate the ParentWeb variable. Creating an extra instance would make the API less efficient. On that basis, I personally am happy to write code in this way. If you think I've got this wrong (I can't guarantee I haven't), of course I'd love to hear from you.

In an upcoming article, I'll also discuss the idea of going beyond the MSDN examples, and trying to write SharePoint data access code in a more 'enterprise' or 'patterns' kind of way. Stay tuned for more on this.


Disposing SharePoint objects - required reading

4 comments:

Anonymous said...

A part of the problem is that Microsoft uses the IDisposable interface inconsistently. SPWeb implements it, but SPList doesn't, even though both can contain a reference to a SPRequest object. And that's not the only case where a class that doesn't implement IDisposable can create objects that do require disposing. While you could use Reflector to find out which methods cause an SPRequest to be instantiated, you shouldn't have to rely on implementation details.

You can avoid this somewhat by instatiating any SPWebs and SPSites that you might need early and dispose those at the very last moment, but that means you'll be holding onto objects longer than necessary and using more resources than you need while your code is running. It still beats the alternative, though.

Anonymous said...

Chris ~

I've posted a possible alternative approach here. Let me know what you think.

Cheers ~
Keith

tojonas said...

Thanks for a good post.

The problem is that this doesn't scale at all.

You have already touched on the problem of writing LIBRARY code for SharePoint, and that is what I'm talking about below.

In my opinion it's IMPOSSIBLE to do it "correct" since sometimes you should dispose of objects and sometimes you shouldn't. (Remember that I'm talking about library code where the method has no knowledge about how the SPWeb and SPSite was constructed)


The deeper your call stack gets the more impossible it gets.

In your example you only have two levels. Your main method and then you get the objects using two helper methods. And you had to intoduce a flag to know when to dispose or not.

Just imagine that you have more nested calls and you see it can't be done.

The reason I say it can't be done is that we have been told that just by accessing some properties of the OM new SPWeb objects have been created and it's UP TO US as clients of the OM to dispose of these.

And since the OM is caching all the references and returning the same instance (as you showed with your GetHashCode() sample) we can't even DO this since we might dispose of an object that are later being used in another method.

When we call dispose on the object the ObjectModel doesn't know about this so it's internal member is still set to the same object but NOW THIS OBJECT is disposed !!!

The OM is unfortunately broken.

The only safe way is to NEVER dispose of any objects UNLESS you have explicitly created them yourself.

I'm considering writing a facade ontop of the OM that will reference count everything returned and the client has to dispose of EVERYTHING EVERYTIME and it's up to this layer to now when to actually call dispose.

Thanks
/Jonas

Chris O'Brien said...

Interesting comments - these are good contributions, thanks.

Keith - I like your idea, it's a nice alternative to my suggestion. One drawback however is it works well for a small call stack (like my example), but similar to Tojonas' comment, not so well for a more complex pattern - you'd need to pass your out parameter around between methods/classes. This is avoided to a certain extent in my structure, where you only need the SPList/SPListItem which you could be passing around for actual use.

Still, I'm not sure either is a real solution to the deeper problem Jonas points out very well. I'm not sure I'd go as far as to say the OM is broken, but it sure is tricky to write library code around the API. Jonas, if you give any further consideration to your layer I'd be interested to hear more, but I can also see there could be interesting issues around class responsiblity there.

I have a follow-up post which expands on these kind of issues soon.

Chris.