Skip to content

Commit

Permalink
Changed behavior of unique_resource constructor to be non-destructive.
Browse files Browse the repository at this point in the history 
If the resource type is nothrow move-constructible and the deleter type
is not, and the deleter's copy constructor throws during unique_resource
move constructor, then move the move-constructed resource back to the
original object instead of invoking the deleter on it. This leaves the
move source in its original state rather than in unallocated state,
which means unique_resource move constructor now provides strong
exception guarantee.
  • Loading branch information
@Lastique
Lastique committed Jan 28, 2024
1 parent 6f2adf3 commit a064912
Show file tree
Hide file tree
Showing 4 changed files with 298 additions and 43 deletions.
3 changes: 3 additions & 0 deletions doc/changelog.qbk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ Updates according to Boost [@https://lists.boost.org/Archives/boost/2024/01/2557
* Added documentation sections describing differences between Boost.Scope and Library Fundamentals TS (see
[link scope.scope_guards.comparison_with_library_fundamentals_ts here] and
[link scope.unique_resource.comparison_with_library_fundamentals_ts here]).
* [link scope.unique_resource `unique_resource`] move constructor was modified to preserve the original state of the
source argument in case of exception. This deviates from the TS behavior, which specifies to invoke the deleter on
the move-constructed resource, but it means the move constructor now maintains strong exception guarantee.

[heading 0.1]

Expand Down
10 changes: 9 additions & 1 deletion doc/unique_resource.qbk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ the limitation in some cases, as shown above.

[section:constructors Constructor differences]

Unlike Library fundamentals TS, Boost.Scope does not consider pointers to functions default-constructible for the purpose of deleters specified
Unlike Library Fundamentals TS, Boost.Scope does not consider pointers to functions default-constructible for the purpose of deleters specified
in `unique_resource` template parameters. For example:

std::experimental::unique_resource< int, int (*)(int) > fd1; // compiles, creates unusable unique_resource object
Expand Down Expand Up @@ -438,6 +438,14 @@ call the deleter on destruction. `default_resource` also works with resource tra
In the above case, the `fd` object will use the default value specified by the resource traits (i.e. -1, in case of fd_resource_traits) to initialize
its stored resource object. Again, the `fd` object is in an unallocated state after construction.

Library Fundamentals TS specifies that `unique_resource` move constructor must deallocate the resource if the resource is nothrow move-constructible
and the deleter is not and the deleter's copy constructor throws and exception. This is a case when `unique_resource` is half-constructed: the
resource has been moved into the object being constructed (i.e. is no longer stored in the source object) but the deleter fails to copy-construct
(i.e. it only exists in the source object). The behavior described in the TS ensures that the resource doesn't leak, but it leaves the source
`unique_resource` unallocated in case of exception, which means the move constructor only maintains
[@https://en.cppreference.com/w/cpp/language/exceptions#Exception_safety basic exception guarantee]. Boost.Scope in this case leaves the source
`unique_resource` in its original state, which also guarantees that the resource doesn't leak, but provides a strong exception guarantee.

[endsect]

[section:check_allocated Checking whether resource is allocated]
Expand Down
198 changes: 159 additions & 39 deletions include/boost/scope/unique_resource.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#ifndef BOOST_SCOPE_UNIQUE_RESOURCE_HPP_INCLUDED_
#define BOOST_SCOPE_UNIQUE_RESOURCE_HPP_INCLUDED_

#include <new> // for placement new
#include <type_traits>
#include <boost/core/addressof.hpp>
#include <boost/core/invoke_swap.hpp>
Expand Down Expand Up @@ -172,16 +173,109 @@ struct has_deallocated_state_impl
template< typename Resource, typename Traits >
struct has_deallocated_state : public has_deallocated_state_impl< Resource, Traits >::type { };

template< typename Resource, typename Traits, bool = has_custom_default< Resource, Traits >::value >
class resource_holder :
template< typename Resource, bool = std::is_nothrow_move_assignable< typename wrap_reference< Resource >::type >::value >
class resource_storage
{
public:
typedef Resource resource_type;
typedef typename wrap_reference< Resource >::type internal_resource_type;

private:
// Note: Not using compact_storage since we will need to reuse storage for this complete object in move_from
internal_resource_type m_resource;

public:
template<
bool Requires = std::is_default_constructible< internal_resource_type >::value,
typename = typename std::enable_if< Requires >::type
>
constexpr resource_storage() noexcept(std::is_nothrow_default_constructible< internal_resource_type >::value) :
m_resource()
{
}

template<
typename R,
typename = typename std::enable_if< std::is_constructible< internal_resource_type, R >::value >::type
>
explicit resource_storage(R&& res) noexcept(std::is_nothrow_constructible< internal_resource_type, R >::value) :
m_resource(static_cast< R&& >(res))
{
}

internal_resource_type& get() noexcept
{
return m_resource;
}

internal_resource_type const& get() const noexcept
{
return m_resource;
}

void move_from(internal_resource_type&& that)
noexcept(std::is_nothrow_constructible< internal_resource_type, typename detail::move_or_copy_construct_ref< resource_type >::type >::value)
{
// Use destroy + construct sequence. Since move_from is only called when we know the constructor doesn't throw,
// this sequence is also guaranteed not to throw.
internal_resource_type* p = boost::addressof(m_resource);
p->~internal_resource_type();
new (p) internal_resource_type(static_cast< typename detail::move_or_copy_construct_ref< resource_type >::type >(that));
}
};

template< typename Resource >
class resource_storage< Resource, true > :
public detail::compact_storage< typename wrap_reference< Resource >::type >
{
public:
typedef Resource resource_type;
typedef typename wrap_reference< Resource >::type internal_resource_type;
typedef Traits traits_type;

private:
typedef detail::compact_storage< internal_resource_type > resource_base;

public:
template<
bool Requires = std::is_default_constructible< internal_resource_type >::value,
typename = typename std::enable_if< Requires >::type
>
constexpr resource_storage() noexcept(std::is_nothrow_default_constructible< internal_resource_type >::value) :
resource_base()
{
}

template<
typename R,
typename = typename std::enable_if< std::is_constructible< internal_resource_type, R >::value >::type
>
explicit resource_storage(R&& res) noexcept(std::is_nothrow_constructible< internal_resource_type, R >::value) :
resource_base(static_cast< R&& >(res))
{
}

using resource_base::get;

void move_from(internal_resource_type&& that) noexcept
{
resource_base::get() = static_cast< internal_resource_type&& >(that);
}
};

template< typename Resource, typename Traits, bool = has_custom_default< Resource, Traits >::value >
class resource_holder :
public detail::resource_storage< Resource >
{
public:
typedef Resource resource_type;

private:
typedef detail::resource_storage< resource_type > resource_base;

public:
typedef typename resource_base::internal_resource_type internal_resource_type;
typedef Traits traits_type;

public:
template<
bool Requires = std::is_default_constructible< internal_resource_type >::value,
Expand Down Expand Up @@ -252,13 +346,17 @@ class resource_holder :

template< typename Resource, typename Traits >
class resource_holder< Resource, Traits, true > :
public detail::compact_storage< typename wrap_reference< Resource >::type >
public detail::resource_storage< Resource >
{
public:
typedef Resource resource_type;
typedef typename wrap_reference< Resource >::type internal_resource_type;

private:
typedef detail::resource_storage< resource_type > resource_base;

public:
typedef typename resource_base::internal_resource_type internal_resource_type;
typedef Traits traits_type;
typedef detail::compact_storage< internal_resource_type > resource_base;

public:
constexpr resource_holder()
Expand Down Expand Up @@ -336,6 +434,8 @@ class deleter_holder :
typedef Resource resource_type;
typedef Deleter deleter_type;
typedef typename wrap_reference< deleter_type >::type internal_deleter_type;

private:
typedef detail::compact_storage< internal_deleter_type > deleter_base;

public:
Expand Down Expand Up @@ -465,6 +565,7 @@ class unique_resource_data :
unique_resource_data
(
static_cast< unique_resource_data&& >(that),
typename std::is_nothrow_constructible< internal_resource_type, typename detail::move_or_copy_construct_ref< resource_type >::type >::type(),
typename std::is_nothrow_constructible< internal_deleter_type, typename detail::move_or_copy_construct_ref< deleter_type >::type >::type()
)
{
Expand Down Expand Up @@ -606,31 +707,42 @@ class unique_resource_data :
}

private:
unique_resource_data(unique_resource_data&& that, std::true_type)
noexcept(std::is_nothrow_constructible< internal_resource_type, typename detail::move_or_copy_construct_ref< resource_type >::type >::value) :
unique_resource_data(unique_resource_data&& that, std::true_type, std::true_type) noexcept :
resource_holder(static_cast< typename detail::move_or_copy_construct_ref< resource_type >::type >(that.get_resource())),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter()), resource_holder::get(), false),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter())),
m_allocated(that.m_allocated)
{
that.m_allocated = false;
}

unique_resource_data(unique_resource_data&& that, std::false_type) try :
unique_resource_data(unique_resource_data&& that, std::false_type, std::true_type) :
resource_holder(static_cast< resource_type const& >(that.get_resource())),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter())),
m_allocated(that.m_allocated)
{
that.m_allocated = false;
}

unique_resource_data(unique_resource_data&& that, std::true_type, std::false_type) try :
resource_holder(static_cast< typename detail::move_or_copy_construct_ref< resource_type >::type >(that.get_resource())),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter()), resource_holder::get(),
std::is_nothrow_constructible< internal_resource_type, resource_type&& >::value && that.m_allocated), // don't deallocate if the resource was copy-constructed
deleter_holder(static_cast< deleter_type const& >(that.get_deleter())),
m_allocated(that.m_allocated)
{
that.m_allocated = false;
}
catch (...)
{
BOOST_IF_CONSTEXPR (std::is_nothrow_constructible< internal_resource_type, resource_type&& >::value)
{
// The resource was moved to this object, and the deleter constructor failed with an exception.
// The deleter holder has invoked the deleter already, so the move source is no longer valid.
that.m_allocated = false;
}
// Since only the deleter could have thrown an exception here, move the resource back
// to the original unique_resource. This is guaranteed to not throw.
that.resource_holder::move_from(static_cast< internal_resource_type&& >(get_internal_resource()));
}

unique_resource_data(unique_resource_data&& that, std::false_type, std::false_type) :
resource_holder(static_cast< resource_type const& >(that.get_resource())),
deleter_holder(static_cast< deleter_type const& >(that.get_deleter())),
m_allocated(that.m_allocated)
{
that.m_allocated = false;
}

void assign(unique_resource_data&& that, std::true_type)
Expand Down Expand Up @@ -711,6 +823,7 @@ class unique_resource_data< Resource, Deleter, Traits, true > :
unique_resource_data
(
static_cast< unique_resource_data&& >(that),
typename std::is_nothrow_constructible< internal_resource_type, typename detail::move_or_copy_construct_ref< resource_type >::type >::type(),
typename std::is_nothrow_constructible< internal_deleter_type, typename detail::move_or_copy_construct_ref< deleter_type >::type >::type()
)
{
Expand Down Expand Up @@ -848,29 +961,38 @@ class unique_resource_data< Resource, Deleter, Traits, true > :
}

private:
unique_resource_data(unique_resource_data&& that, std::true_type)
noexcept(std::is_nothrow_constructible< internal_resource_type, typename detail::move_or_copy_construct_ref< resource_type >::type >::value) :
unique_resource_data(unique_resource_data&& that, std::true_type, std::true_type) noexcept :
resource_holder(static_cast< typename detail::move_or_copy_construct_ref< resource_type >::type >(that.get_resource())),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter()), resource_holder::get(), false)
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter()))
{
that.set_deallocated();
}

unique_resource_data(unique_resource_data&& that, std::false_type, std::true_type) :
resource_holder(static_cast< resource_type const& >(that.get_resource())),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter()))
{
that.set_deallocated();
}

unique_resource_data(unique_resource_data&& that, std::false_type) try :
unique_resource_data(unique_resource_data&& that, std::true_type, std::false_type) try :
resource_holder(static_cast< typename detail::move_or_copy_construct_ref< resource_type >::type >(that.get_resource())),
deleter_holder(static_cast< typename detail::move_or_copy_construct_ref< deleter_type >::type >(that.get_deleter()), resource_holder::get(),
std::is_nothrow_constructible< internal_resource_type, resource_type&& >::value && is_allocated()) // don't deallocate if the resource was copy-constructed
deleter_holder(static_cast< deleter_type const& >(that.get_deleter()))
{
that.set_deallocated();
}
catch (...)
{
BOOST_IF_CONSTEXPR (std::is_nothrow_constructible< internal_resource_type, resource_type&& >::value)
{
// The resource was moved to this object, and the deleter constructor failed with an exception.
// The deleter holder has invoked the deleter already, so the move source is no longer valid.
that.set_deallocated();
}
// Since only the deleter could have thrown an exception here, move the resource back
// to the original unique_resource. This is guaranteed to not throw.
that.resource_holder::move_from(static_cast< internal_resource_type&& >(get_internal_resource()));
}

unique_resource_data(unique_resource_data&& that, std::false_type, std::false_type) :
resource_holder(static_cast< resource_type const& >(that.get_resource())),
deleter_holder(static_cast< deleter_type const& >(that.get_deleter()))
{
that.set_deallocated();
}

template<
Expand Down Expand Up @@ -1227,12 +1349,10 @@ class unique_resource
* otherwise copy-constructs. If \c Deleter is nothrow move-constructible then move-constructs
* \c Deleter, otherwise copy-constructs. Deactivates the moved-from unique resource object.
*
* If `that.allocated()` was \c true prior to the operation and constructing \c Deleter
* throws after \c Resource is move-constructed, invokes the original deleter stored in \a that
* on the resource and deactivates \a that before returning with the exception. In other
* exceptional cases \a that is left intact.
* If an exception is thrown during construction, \a that is left in its original state.
*
* \note This logic ensures that the resource is not leaked in case of an exception.
* \note This logic ensures that in case of exception the resource is not leaked and remains owned by the
* move source.
*
* **Throws:** Nothing, unless construction of \c Resource or \c Deleter throws.
*
Expand Down Expand Up @@ -1261,10 +1381,10 @@ class unique_resource
* the \c Deleter object first and the \c Resource object next. Otherwise, move-assigns
* the objects in reverse order. Lastly, deactivates the moved-from unique resource object.
*
* If an exception is thrown, \a that is left intact.
* If an exception is thrown, \a that is left in its original state.
*
* \note The different orders of assignment ensure that in case of an exception the resource is not leaked
* and ramains owned by the move source.
* \note The different orders of assignment ensure that in case of exception the resource is not leaked
* and remains owned by the move source.
*
* **Throws:** Nothing, unless assignment of \c Resource or \c Deleter throws.
*
Expand Down Expand Up @@ -1461,7 +1581,7 @@ class unique_resource
* **Requires:** \c Resource and \c Deleter are swappable. At least one of \c Resource and \c Deleter
* is nothrow swappable.
*
* **Effects:** Swaps the resource objects and deleter objects stored in `*this` and `that`
* **Effects:** Swaps the resource objects and deleter objects stored in `*this` and \a that
* as if by calling unqualified `swap` in a context where `std::swap` is
* found by overload resolution.
*
Expand Down
Loading

0 comments on commit a064912

Please sign in to comment.