4

I was happy enough to have inherited a terribly written SharePoint project.
Apparently, the original developer was a big fan of reusable code (30% of code is reused across 20 projects without using any libraries—guess how?).

I would often find his code calling some Common.OpenWeb method to retrieve SPWeb object for operating SharePoint stuff. Most of this function's incarnations look exactly the same:

public SPWeb OpenWeb()
{
    String strSiteUrl = ConfigurationManager.AppSettings["SiteUrl"].ToString();
    SPSite site = null;
    SPWeb web = null;
    try
    {
        using (site = new SPSite(strSiteUrl))
        {
            using (web = site.OpenWeb())
            {
                return web;
            }
        }
    }
    catch (Exception ex)
    {
        LogEvent("Error occured in OpenWeb : " + ex.Message, EventLogEntryType.Error);
    }
    return web;
}

And now I'm really worried.
How come this works in production? This method always returns a disposed object, right?

How unstable is it, exactly?

UPDATE:

This method is used in the following fashion:

oWeb = objCommon.OpenWeb();
SPList list = oWeb.Lists["List name"];
SPListItem itemToAdd = list.Items.Add();
itemToAdd["Some field"] = "Some value";
oWeb.AllowUnsafeUpdates = true;
itemToAdd.Update();
oWeb.AllowUnsafeUpdates = false;

I omitted the swallowing try-catch for brevity.
This code inserts value into the list! This is a write operation, I'm pretty sure Request property is being used for this. Then how can it work?

Dan Abramov
  • 264,556
  • 84
  • 409
  • 511
  • Not only is it unstable for returning disposed objects, I note that it logs and then ignores any exceptions. – John Saunders Feb 14 '11 at 20:20
  • 2
    At least it will not be very memory hungry – rene Feb 14 '11 at 20:21
  • The method indeed returns a disposed object. All calls on that object should theoretically throw `ObjectDisposedException`. I suspect it never worked, or that no calls are ever made on those disposed objects. – Frédéric Hamidi Feb 14 '11 at 20:21
  • That's just one of those sweet moments about this project. Each method has its own `catch` that logs something somewhere (in lucky case) and then goes on. Classic Pokemon Programming (see http://blog.whiletrue.com/2011/01/what-if-visual-studio-had-achievements/) – Dan Abramov Feb 14 '11 at 20:23
  • @Frédéric Hamidi, this code is in production and it *does* work, although due to bad logging it's impossible now to see how often it fails. – Dan Abramov Feb 14 '11 at 20:25
  • I updated the question to reflect your comment, thanks. – Dan Abramov Feb 14 '11 at 20:40

1 Answers1

12

First, the short answer: that method indeed returns a disposed object. An object should not be used after being disposed, because it's no longer in a reliable state, and any further operation performed on that object should (theoretically) throw an ObjectDisposedException.

Now, after digging a little, SharePoint objects don't seem to follow that rule. Not only does SPWeb never throw ObjectDisposedException after being disposed, but it actually tests for that case in its Request property and rebuilds a valid SPRequest from its internal state if it has been disposed.

It seems that at least SPWeb was designed to be fully functional even in a disposed state. Why, I don't know. Maybe it's for accommodating client code like the one you're working on. Maybe it's some kind of complicated optimization I can't possibly understand.

That said, I'd suggest you don't rely on that behavior, because it might change in the future (even though, given Microsoft's policy on bug-for-bug backwards compatibility, it might not).

And of course, you will still leak the new SPRequest instance, which can be quite costly. Never, ever, use a disposed object, even if SharePoint lets you get away with it.

Frédéric Hamidi
  • 258,201
  • 41
  • 486
  • 479
  • Thanks a lot for descriptive answer. – Dan Abramov Feb 14 '11 at 21:09
  • none of exceptions should thrown if you use disposed object. The main thing is while you hold reference to SPweb in any variable, garbage collector unable to perform memory cleanup and huge amount of memory may be used – gdbdable Dec 30 '13 at 12:17
  • The main reason it rebuilds itself is because traditionally, the dispose guidance for sharepoint has been woefully bad, utterly confusing and often downright incorrect. This is a legacy hack to do the "right thing" when all else fails. Yes, it stinks. – x0n Jan 30 '14 at 22:19