diff --git a/doc/changelog.qbk b/doc/changelog.qbk index c072561..291aa7f 100644 --- a/doc/changelog.qbk +++ b/doc/changelog.qbk @@ -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] diff --git a/doc/unique_resource.qbk b/doc/unique_resource.qbk index b7b82b2..8bdf95e 100644 --- a/doc/unique_resource.qbk +++ b/doc/unique_resource.qbk @@ -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 @@ -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] diff --git a/include/boost/scope/unique_resource.hpp b/include/boost/scope/unique_resource.hpp index 19301a0..5211183 100644 --- a/include/boost/scope/unique_resource.hpp +++ b/include/boost/scope/unique_resource.hpp @@ -14,6 +14,7 @@ #ifndef BOOST_SCOPE_UNIQUE_RESOURCE_HPP_INCLUDED_ #define BOOST_SCOPE_UNIQUE_RESOURCE_HPP_INCLUDED_ +#include // for placement new #include #include #include @@ -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, @@ -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() @@ -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: @@ -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() ) { @@ -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) @@ -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() ) { @@ -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< @@ -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. * @@ -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. * @@ -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. * diff --git a/test/run/unique_resource.cpp b/test/run/unique_resource.cpp index dc4e504..cadef23 100644 --- a/test/run/unique_resource.cpp +++ b/test/run/unique_resource.cpp @@ -910,6 +910,69 @@ struct wrapped_int_resource_traits } }; +// Specially-crafted resource type that is: +// * nothrow move-constructible +// * NOT-nothrow move-assignable +// * has data layout which favors placing the "allocated" flag in unique_resource in its tail padding +class move_constructible_resource +{ +private: + int m_n1; + signed char m_n2; + +public: + constexpr move_constructible_resource() noexcept : m_n1(0), m_n2(0) {} + explicit move_constructible_resource(int n1, signed char n2) noexcept : m_n1(n1), m_n2(n2) {} + + move_constructible_resource(move_constructible_resource&& that) noexcept : m_n1(that.m_n1), m_n2(that.m_n2) + { + that.m_n1 = 0; + that.m_n2 = 0; + } + + move_constructible_resource(move_constructible_resource const& that) : m_n1(that.m_n1), m_n2(that.m_n2) + { + } + + move_constructible_resource& operator= (move_constructible_resource&& that) // not noexcept + { + m_n1 = that.m_n1; + m_n2 = that.m_n2; + that.m_n1 = 0; + that.m_n2 = 0; + return *this; + } + + move_constructible_resource& operator= (move_constructible_resource const& that) + { + m_n1 = that.m_n1; + m_n2 = that.m_n2; + return *this; + } + + bool operator== (move_constructible_resource const& that) const noexcept + { + return m_n1 == that.m_n1 && m_n2 == that.m_n2; + } + + bool operator!= (move_constructible_resource const& that) const noexcept + { + return !operator==(that); + } + + friend std::ostream& operator<< (std::ostream& strm, move_constructible_resource const& res) + { + strm << "{ " << res.m_n1 << ", " << static_cast< int >(res.m_n2) << " }"; + return strm; + } + + friend void copy_resource(move_constructible_resource const& from, move_constructible_resource& to) + { + to.m_n1 = from.m_n1; + to.m_n2 = from.m_n2; + } +}; + template< template< typename > class Traits > void check_throw_deleter() { @@ -1030,9 +1093,10 @@ void check_throw_deleter() } catch (...) { - // The resource was moved from ur1, but the deleter could not have been moved and was called on the moved resource - BOOST_TEST_EQ(n, 1); - BOOST_TEST_EQ(deleted_res1, moveable_resource{ 10 }); + // The resource was moved from ur1, but the deleter could not have been moved. Then the resource was moved back to ur1. + BOOST_TEST_EQ(n, 0); + BOOST_TEST(ur1.allocated()); + BOOST_TEST_EQ(ur1.get(), moveable_resource{ 10 }); throw; } } @@ -1040,6 +1104,7 @@ void check_throw_deleter() { } BOOST_TEST_EQ(n, 1); + BOOST_TEST_EQ(deleted_res1, moveable_resource{ 10 }); } n = 0; @@ -1072,6 +1137,65 @@ void check_throw_deleter() BOOST_TEST_EQ(deleted_res1, moveable_resource{ 10 }); BOOST_TEST_EQ(deleted_res2, moveable_resource{ 20 }); } + + n = 0; + { + move_constructible_resource deleted_res1; + try + { + typedef boost::scope::unique_resource< move_constructible_resource, throwing_resource_deleter< move_constructible_resource >, Traits< move_constructible_resource > > unique_resource_t; + unique_resource_t ur1{ move_constructible_resource(10, 5), throwing_resource_deleter< move_constructible_resource >(deleted_res1, n) }; + ur1.get_deleter().set_throw(true); + try + { + unique_resource_t ur2 = std::move(ur1); + BOOST_ERROR("An exception is expected to be thrown by throwing_resource_deleter"); + } + catch (...) + { + // The resource was moved from ur1, but the deleter could not have been moved. Then the resource was moved back to ur1. + BOOST_TEST_EQ(n, 0); + BOOST_TEST(ur1.allocated()); + BOOST_TEST_EQ(ur1.get(), move_constructible_resource(10, 5)); + throw; + } + } + catch (...) + { + } + BOOST_TEST_EQ(n, 1); + BOOST_TEST_EQ(deleted_res1, move_constructible_resource(10, 5)); + } + + n = 0; + { + move_constructible_resource deleted_res1; + try + { + // No resource traits to force using the "allocated" flag + typedef boost::scope::unique_resource< move_constructible_resource, throwing_resource_deleter< move_constructible_resource > > unique_resource_t; + unique_resource_t ur1{ move_constructible_resource(10, 0), throwing_resource_deleter< move_constructible_resource >(deleted_res1, n) }; + ur1.get_deleter().set_throw(true); + try + { + unique_resource_t ur2 = std::move(ur1); + BOOST_ERROR("An exception is expected to be thrown by throwing_resource_deleter"); + } + catch (...) + { + // The resource was moved from ur1, but the deleter could not have been moved. Then the resource was moved back to ur1. + BOOST_TEST_EQ(n, 0); + BOOST_TEST(ur1.allocated()); + BOOST_TEST_EQ(ur1.get(), move_constructible_resource(10, 0)); + throw; + } + } + catch (...) + { + } + BOOST_TEST_EQ(n, 1); + BOOST_TEST_EQ(deleted_res1, move_constructible_resource(10, 0)); + } }