Smart pointers are too smart

Joel’s law of leaky abstractions rears its ugly head once more. I try to never use smart pointers because… I’m not smart enough.

COM programmers are of course intimately familiar with AddRef and Release. The designers of COM decided to use reference counting as the mechanism for implementing storage management in COM, and decided to put the burden of implementing and calling these methods upon the users.

It is very natural when using a language like C++ to say that perhaps we can encapsulate these semantics into an object, and let the C++ compiler worry about calling the AddRefs
and Releases in the appropriate constructors, copy constructors and destructors. It is very natural and very tempting, and I avoid template libraries that do so like the plague.

Everything usually works fine until there is some weird, unforeseen interaction between the various parts. Let me give you an example: Suppose you have the following situation: you have a C++ template library map mapping IFoo pointers onto IBar pointers, where the pointers to the IFoo and IBar are in smart pointers. You want the map to take ownership of the pointers. Does this code look correct?

map[srpFoo.Disown()] = srpBar.Disown();

It sure looks correct, doesn’t it?

Look again. Is there a memory leak there?

I found code like this in a library I was maintaining once, and since I had never used smart pointer templates before, I decided to look at what exactly this was doing at the assembly level. A glance at the generated assembly shows that the order of operations is:

1) call srpFoo.Disown() to get the IFoo*

2) call srpBar.Disown() to get the IBar*

3) call map‘s operator[] passing in the IFoo*, returning an IBar**

4) do the assignment of the IBar* to the address returned in (3).

So where is the leak? This library had C++ exception handling for out-of-memory exceptions turned on. If the operator[] throws an out-of-memory exception then the not-smart IFoo* and IBar* presently on the argument stack are both going to leak.

The correct code is to copy the pointers before you disown them:

map[srpFoo] = srpBar;
srpFoo.Disown();
srpBar.Disown();

Before the day that I found this I had never been in a situation before where I had to think about the C++ order of operations for assigning and subscripting in order to get the error handling right! The fact that you have to know these picky details about C++ operator semantics in order to get the error handling right is an indication that people are going to get it wrong.

Let me give you another example. One day I was adding a feature to this same library, and I noticed that I had caused a memory leak. Clearly I must have forgotten to call Release somewhere, or perhaps some smart pointer code was screwed up. I figured I’d just put a breakpoint on the AddRef and Release for the object, and figure out who was calling the extra AddRef.

Here — and I am not making this up! — is the call stack at the point of the first AddRef:

ATL::CComPolyObject<CLayMgr>::AddRef
ATL::CComObjectRootBase::OuterAddRef
ATL::CComContainedObject<CLayMgr>::AddRef
ATL::AtlInternalQueryInterface
ATL::CComObjectRootBase::InternalQueryInterface
CLayMgr::_InternalQueryInterface
ATL::CComPolyObject<CLayMgr>::QueryInterface
ATL::CComObjectRootBase::OuterQueryInterface
ATL::CComContainedObject<CLayMgr>::QueryInterface
CDDS::FinalConstruct
ATL::CComPolyObject<CDDS>::FinalConstruct
ATL::CComCreator<ATL::CComPolyObject<CDDS>>::CreateInstance
CTryAssertComCreator<ATL::CComPolyObject<CDDS>>::CreateInstance
ATL::CComClassFactory2<CDLic>::CreateInstance(ATL::CComClassFactory2
CTryAssertClassFactory2<CDLic>::CreateInstance(CTryAssertClassFactory2<CDLic>

Good heavens!

Maybe you ATL programmers out there are smarter than me. In fact, I am almost certain you are, because I have not the faintest idea what the differences between a CComContainedObject, a CComObjectRootBase and a CComPolyObject are! It took me hours to debug this memory leak. So much for smart pointers saving time!

I am too dumb to understand that stuff, so when I write COM code, my implementation of AddRef is one line long, not hundreds of lines of dense macro-ridden, templatized cruft winding its way through half a dozen wrapper classes.


Some reader responses:

Do you really think [the original example] is a normal way of using smart pointers? It is not.

I have no idea if it is “normal” or not, but it is how I found the code that I was reading, and there was plenty more code like it. Saying “you’re doing it wrong” doesn’t help. If the easy, obvious way to do a thing, and the correct, safe way to do a thing are different, then we have a problem.

You don’t have to use or understand things like CComObjectRootBase if all you want is a smart pointer.

Well, first off, I don’t want smart pointers. Second, again, this was not a hole of my own digging here. I was asked to help out with an existing codebase that used smart pointers, and debugging memory leaks in it really slowed me down.

The real problem is that exceptions were bolted on to C++ late in the process.

I agree that exceptions are a poor fit for non-GC’d languages, but again, that’s the world we live in, and we should adapt our tools to reality, rather than building tools that make real-world scenarios more difficult to analyze.

Using smart pointers is almost always a better idea than trying to manage the refcounts “by hand”.

I thoroughly disagree. I’ve debugged ref count bugs caused by me forgetting an AddRef, and I’ve debugged ref count bugs like the one above with the fifteen-deep call stack of meaningless (to me) gibberish, and the former are a whole lot easier to track down.

When I write COM code it tends to emphasize the error paths and cleanup code rather than the program logic, and that’s unfortunate. But given that the error paths and cleanup code are where most of the bugs are, I want them to be as explicit as possible. Those are the program logic.

I understand the urge to abstract them away into classes that take care of the details for you. But in my experience, the abstraction is both complex and leaky. The abstraction is easy to use for simple cases, and thereby affords creation of much more subtle, hard-to-debug bugs.

My point, simply, is that AddRef, Release and QueryInterface are a pain in the rear to use, but they are easy to understand. Smart pointers are easy to use and hard to understand, and I prefer to understand the code I write.

So basically you are saying smart pointers suck, because you don’t know how to use them properly?

No, I’m saying they suck because they are easy to accidentally use improperly, and because they slow down my ability to understand and debug a buggy codebase.

An abstraction is supposed to relieve you of the burden of understanding the abstracted thing, but smart pointers do a poor job of that. In order to use them correctly, you have to understand exactly what they’re doing to ensure that you don’t use smart pointers to violate the very rules they are designed to abstract away.

That’s a lousy abstraction. It is easier for me to learn the rules of COM and apply them than to learn the rules of COM and learn the entire smart pointer framework.

The .NET framework does a much better job of abstracting away the details of how the underlying system works, because it was designed to do that from day one. Smart pointers are a kludge written post hoc.

You have bad smart pointers! Don’t blame a concept because of a lame implementation!

Again, I’m not the one choosing what implementation of smart pointers I get to use. Maybe there are smart pointer libraries that do not have these problems, but I’m betting they just have different problems.

This code has exactly the same problem, and no smart pointers: map[new Foo()] = new Bar();

Sure, and I can look at that code and immediately know that it is wrong without having to understand the semantics of “disown” operations. The only thing I need to know to deduce the wrongness of that code is the basic rules of C++.

Advertisements

2 thoughts on “Smart pointers are too smart

  1. Pingback: Porting old posts, part 1 | Fabulous adventures in coding

  2. You know, I’m going to recreate some of that old discussion going on. C++ is still a thing, smart pointers are still a thing, and there’s lessons to be learned here.


    Does this code look correct?

    `map[srpFoo.Disown()] = srpBar.Disown();`

    It sure looks correct, doesn’t it?

    It doesn’t to me. It leaves the realm of smart pointers, and therefore I assume it is incorrect until thoroughly proven otherwise. I think the preserved comment got it right when it said:

    This code has exactly the same problem, and no smart pointers: `map[new Foo()] = new Bar();`

    You claim that this code is obviously wrong, while the other code isn’t. I think you were looking at it with a bias of “burnt by raw pointers too often, but these smart pointers promise to make everything easy”. Sure, but only as long as you don’t call methods that turn the smart pointers back into raw pointers. I don’t really understand why you think the first piece of code is correct and the second isn’t, except for the “raw pointers, potential problem” alarm that goes off in a C++ programmer’s head not being tuned to the `Disown()` method.

    So lesson learned: smart pointers can only protect you when you’re actually using them. Once you take a raw pointer out, you’re on your own again. The correct code in this example would be `map[srpFoo] = srpBar;`, with no `Disown()` calls at all. Let the pointers go out of scope naturally. (With move semantics, you might want to write `map[std::move(srpFoo)] = std::move(srpBar);` instead, but that’s a different topic.)

    Your complaint about the ATL COM object call stack is really unfair to smart pointers. The call stack has absolutely nothing to do with smart pointers. Zilch. Not one of those stack frames is in a smart pointer implementation. If you managed your ATL-based COM object with manual AddRef/Release, the call stack would look exactly the same.

    As an aside, your comment about the complexity of this AddRef callstack is also somewhat disingenuous. Most of it is not about AddRef, but about CreateInstance and QueryInterface. Three lines are the actual AddRef, and unlike a typical hand-written 1-line InterlockedIncrement AddRef, this implementation supports COM aggregation.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s