Simplifying code and achieving exception safety using unique_ptr

Recently, I had a bug report against libc++’s implementation of deque.

The code in question was (simplified):

    #ifndef _LIBCPP_NO_EXCEPTIONS
        try
        {
    #endif  // _LIBCPP_NO_EXCEPTIONS
            __buf.push_back(__alloc_traits::allocate(__a, __base::__block_size));
    #ifndef _LIBCPP_NO_EXCEPTIONS
        }
        catch (...)
        {
            __alloc_traits::deallocate(__a, __buf.front(), __base::__block_size);
            throw;
        }
    #endif  // _LIBCPP_NO_EXCEPTIONS

and the bug report read “If the allocation fails, then the pointer never gets
added to __buf, and so the call to deallocate will free the wrong thing.”

Thanks to Tavian Barnes for the bug report and the diagnosis.

There are two operations here that can fail; the first is the allocation,
and the second is the call to push_back. We need to deal with both.

Tavian suggested declaring a variable named __block (since this is allocating
block of memory for the deque to store objects into), and then attempting to
add that to __buf. If that fails, then deallocate __block.

I dismissed this solution immediately, because __block has a special meaning
in Objective-C, and there are people who use libc++ and Objective-C/C++ together.

However, I did try writing with __ablock instead:

    #ifndef _LIBCPP_NO_EXCEPTIONS
        pointer __ablock;
        try
        {
    #endif  // _LIBCPP_NO_EXCEPTIONS
            __ablock = __alloc_traits::allocate(__a, __base::__block_size);
            __buf.push_back(__ablock);
    #ifndef _LIBCPP_NO_EXCEPTIONS
        }
        catch (...)
        {
            __alloc_traits::deallocate(__ablock);
            throw;
        }
    #endif  // _LIBCPP_NO_EXCEPTIONS

Testing this, I found that this solved the problem. Yay!
But the code was still ugly. All those ifdefs and the catch (...) bothered me.

Then I remembered unique_ptr, and realized that it did exactly what I wanted.

Libc++ has an internal class called __allocator_destructor, whose constructor
takes an allocator (by reference) and a size_t, and whose operator() takes a
pointer and deletes it, using the allocator’s deallocate function (passing in
the size). This is used in several places in libc++.

Suddenly, the code got much simpler:

    typedef __allocator_destructor<_Allocator> _Dp;
    unique_ptr<pointer, _Dp> __hold(
        __alloc_traits::allocate(__a, __base::__block_size),
            _Dp(__a, __base::__block_size));
    __buf.push_back(__hold.get());
    __hold.release();

This looks very different – what’s going on here?

This code allocates the block of memory and immediately stuffs it into a
unique_ptr (__hold). Then it adds it to __buf as before. Then we release
it from __hold, which says that it doesn’t need to be deallocated when __hold
goes out of scope.

What about when an exception is thrown?
* If the allocation throws, the __hold is never constructed. Nothing was
allocated, and nothing needs to be deleted.
* If the push_back throws, then __hold‘s destructor deallocates the memory for us.

In either case, the right thing happens.

The general pattern for this is:

    unique_ptr<T> holder(new T{/* params */});
    // set up some stuff that uses holder.get()
    holder.release();

If the “stuff” throws, then holder cleans up. Simple, and no need for try/catch
blocks to handle deallocations.

If you’re writing code that does allocations and other operations that can throw,
this pattern could simplify your code a lot.

Advertisements

17 thoughts on “Simplifying code and achieving exception safety using unique_ptr

    1. Marshall Clow Post author

      make_unique does not let you specify a custom deleter, so in this case it wouldn’t work.

      In general, you can use make_unique; but since it’s just a thin wrapper, the only advantage is clarity of code. [make_shared, on the other hand, has runtime advantages over constructing a shared_ptr yourself ]

      Reply
    1. gmatte

      Because the unique_ptr is used to react to errors. If everything went well the __buf container takes ownership of the allocated resource and the unique_ptr releases it so it won’t be destroyed. __buf is not a container of std::unique_ptrs.

      Reply
    2. vimalthilak

      Hi Marshall

      I have the same question – why call release in this case?

      Can you recommend a pattern for member variables that I’d like to convert over to unique_ptr

      Reply
  1. Rob G

    Personally, I’d avoid using an “internal class” in production code like I’d avoid the plague.

    It’s generally a bad idea to find classes, especially with underscore prefixes, in the implementation of your platform’s libraries and start using them directly in your own code. Stick to the published APIs. Otherwise the result is brittle, non-portable code.

    Reply
  2. jwakely

    Needs more cowbellauto 🙂


    auto __hold = __allocate_raii_blahblah(__a, __base::__block_size);
    __buf.push_back(__hold.get());
    __hold.release();

    Reply
    1. gmatte

      I don’t see the point of using auto o = class{x}; versus class o{x}; Moreover, std::unique_ptr is a perfect and well defined RAII container for automatic deallocation of external (pointed) resource. No need to create a new class for every type of resource management. (Could be a simple using statement to define the std::unique_ptr’s Deleter type, but your post isn’t implying that.)

      Reply
      1. jwakely

        You missed the point. Marshall’s original code had this:


        typedef __allocator_destructor<_Allocator> _Dp;
        unique_ptr<pointer, _Dp> __hold(
        __alloc_traits::allocate(__a, __base::__block_size),
        _Dp(__a, __base::__block_size));

        That needs to repeat the type _Dp three times, and repeat the arguments __a, __base::__block_size twice. It is also cluttered by implementation details that are not essential to see in this context (using unique_ptr with a custom deleter is the solution, but all that’s necessary to know when reading the code is that there is an RAII type managing the memory until it is inserted into the container).

        By writing a function (not a class!!) like __allocate_raii_blahblah all that repetition and clutter goes away, replaced by a single line:

        auto __hold = __allocate_raii_blahblah(__a, __base::__block_size);

      2. jwakely

        And in case it still isn’t obvious, __allocate_raii_blahblah would return the same type as in the original, i.e. unique_ptr<pointer, __allocator_destructor<_Allocator>> so there is no “new class for every type of resource management” because I didn’t change the types, I just wrote an object generator function to help create that type (and of course the function should have a better name).

  3. bbolli

    I think your second version is also wrong: If the allocation fails, __ablock will never be assigned to, but the exception handler will still deallocate it.

    Reply
  4. Zhihao Yuan

    The pattern is fairly common. Modifying the second code block with my `defer` macro (https://github.com/lichray/deferxx):

    “`
    pointer __p = nullptr;
    defer(__alloc_traits::deallocate(__p)) namely(__on_fail);
    __p = __alloc_traits::allocate(__a, __base::__block_size);
    __buf.push_back(__p);
    __on_fail.dismiss();
    “`

    Of course when the resource is a pointer, using `std::unique_ptr` is easier.

    Reply
  5. wilhelmtell

    I think there’s room for a comment on the get() line, to explain why you’re calling get() and not release(). My immediate reaction as I read the code was “replace the get() with release() and you save a line of code!”, but soon realized you only want to release once push_back() completed successfully, and not before then.

    A comment there can save you a regression in the future.

    Reply
  6. Pingback: C++ designing exception classes | DL-UAT

Leave a Reply to Rob G Cancel 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 )

Twitter picture

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

Facebook photo

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

Google+ photo

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

Connecting to %s