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.
In the general pattern wouldn’t it be better to use “std::make_unique”? (If so, why?)
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 ]Why not just let the destructor to free up the resource but calling the release explicitly?
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.
Thanks, this make sense to me now
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
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.
While I agree with you in general, the code that I was fixing is part of the standard library, specifically in libc++’s header.
Needs more
cowbellauto
🙂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.)
You missed the point. Marshall’s original code had this:
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 (usingunique_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);
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).This is, indeed, a nice approach jwakely. It may just be my understanding, but it wasn’t obvious in your first post.
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.
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.
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.
Pingback: C++ designing exception classes | DL-UAT