Skip to content

Commit 7c4e174

Browse files
mingxwamingxwa
authored
Reset the proxy when the destructor of the underlying pointer throws (#82)
* Reset the proxy when the destructor of the underlying pointer throws proxy::destroy() invoked destruction through an lvalue-qualified overload, so the meta_resetting_guard that invoke_impl applies to consuming overloads never ran. When the destructor of the underlying pointer threw, the metadata still pointed at the destroyed object, so has_value() stayed true and the next destruction ran on a dead object. Every path that discards a value reached this: reset(), operator=(nullptr), both branches of the copy assignment operator, the move assignment operator, operator=(P&&) and both emplace overloads. Destruction is a consuming operation like relocation, so give it the same shape. The destroy meta now uses an rvalue-qualified overload, which makes erased_context destroy the pointer through destroying_guard and makes invoke_impl clear the metadata on both the normal and the exceptional path. destroy_dispatch keeps only its tag role and its call operator becomes a no-op, because the destruction it used to perform is what the rvalue machinery already does. The added reset is dead on the non-throwing path and the optimizer removes it. At -O2 the disassembly of ~proxy, reset and the move assignment operator is unchanged for a facade whose destructibility is nothrow. LifetimeTracker gains ThrowingDestructionSession, a Session whose destructor throws, which is the first pointer in the suite with a potentially throwing destructor and the first use of a facade whose destructibility is nontrivial. * Fix MSVC failure
1 parent 07e6520 commit 7c4e174

4 files changed

Lines changed: 96 additions & 13 deletions

File tree

‎include/proxy/v4/detail/core.h‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -385,9 +385,7 @@ R invoke_dispatch(Args&&... args) {
385385
template <class P>
386386
struct destroying_guard {
387387
explicit destroying_guard(P* p) noexcept : p_(p) {}
388-
~destroying_guard() noexcept(std::is_nothrow_destructible_v<P>) {
389-
std::destroy_at(p_);
390-
}
388+
~destroying_guard() noexcept(std::is_nothrow_destructible_v<P>) { p_->~P(); }
391389

392390
private:
393391
P* p_;
@@ -617,10 +615,7 @@ struct copy_dispatch {
617615
}
618616
};
619617
struct destroy_dispatch {
620-
template <class T>
621-
PRO4D_STATIC_CALL(void, T& self) noexcept(std::is_nothrow_destructible_v<T>) {
622-
std::destroy_at(&self);
623-
}
618+
PRO4D_STATIC_CALL(void, auto&&) noexcept {}
624619
};
625620
template <class D, class ONE, class OE, constraint_level C>
626621
struct lifetime_meta_traits : std::type_identity<void> {};
@@ -925,8 +920,8 @@ struct facade_traits : specialization_t<facade_conv_traits_impl,
925920
void(void*) const, F::copyability>,
926921
lifetime_meta_t<relocate_dispatch, void(void*) && noexcept,
927922
void(void*) &&, F::relocatability>,
928-
lifetime_meta_t<destroy_dispatch, void() noexcept, void(),
929-
F::destructibility>,
923+
lifetime_meta_t<destroy_dispatch, void() && noexcept,
924+
void() &&, F::destructibility>,
930925
typename facade_traits::conv_meta,
931926
typename facade_traits::refl_meta>>;
932927
using indirect_accessor = composite_t<
@@ -1331,8 +1326,8 @@ class proxy : public detail::facade_traits<F>::direct_accessor,
13311326
if constexpr (F::destructibility != constraint_level::trivial) {
13321327
if (meta_.has_value()) {
13331328
invoke<detail::destroy_dispatch,
1334-
void() noexcept(F::destructibility ==
1335-
constraint_level::nothrow)>(*this);
1329+
void() && noexcept(F::destructibility ==
1330+
constraint_level::nothrow)>(std::move(*this));
13361331
}
13371332
}
13381333
}

‎include/proxy/v4/detail/proxy_creation.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ template <class Alloc, class T>
7878
void deallocate(const Alloc& alloc, T* ptr) {
7979
auto al =
8080
typename std::allocator_traits<Alloc>::template rebind_alloc<T>(alloc);
81-
std::destroy_at(ptr);
81+
ptr->~T();
8282
al.deallocate(ptr, 1);
8383
}
8484
template <class Alloc>
@@ -223,7 +223,7 @@ class strong_compact_ptr {
223223
strong_compact_ptr(strong_compact_ptr&& rhs) = delete;
224224
~strong_compact_ptr() noexcept(std::is_nothrow_destructible_v<T>) {
225225
if (ptr_->strong_count.fetch_sub(1, std::memory_order::acq_rel) == 1) {
226-
std::destroy_at(operator->());
226+
operator->()->~T();
227227
if (ptr_->weak_count.fetch_sub(1u, std::memory_order::release) == 1) {
228228
deallocate(ptr_->alloc, ptr_);
229229
}

‎tests/proxy_lifetime_tests.cpp‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,14 @@ struct TestTrivialFacade
2626
::support_destruction<pro::constraint_level::trivial> //
2727
::build {};
2828

29+
struct TestThrowingDestructionFacade
30+
: pro::facade_builder //
31+
::add_convention<utils::spec::FreeToString, std::string()> //
32+
::support_copy<pro::constraint_level::nontrivial> //
33+
::support_relocation<pro::constraint_level::nontrivial> //
34+
::support_destruction<pro::constraint_level::nontrivial> //
35+
::build {};
36+
2937
struct TestRttiFacade : pro::facade_builder //
3038
::add_direct_reflection<utils::RttiReflector> //
3139
::add_facade_with_substitution<TestFacade> //
@@ -274,6 +282,21 @@ TEST(ProxyLifetimeTests, TestMoveConstrction_FromNull) {
274282
ASSERT_FALSE(p2.has_value());
275283
}
276284

285+
TEST(ProxyLifetimeTests, TestDestruction_Exception) {
286+
utils::LifetimeTracker tracker;
287+
std::vector<utils::LifetimeOperation> expected_ops;
288+
auto destroy = [&] {
289+
pro::proxy<detail::TestThrowingDestructionFacade> p{
290+
std::in_place_type<utils::LifetimeTracker::ThrowingDestructionSession>,
291+
&tracker};
292+
};
293+
ASSERT_THROW(destroy(), utils::DestructionFailure);
294+
expected_ops.emplace_back(1,
295+
utils::LifetimeOperationType::kValueConstruction);
296+
expected_ops.emplace_back(1, utils::LifetimeOperationType::kDestruction);
297+
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
298+
}
299+
277300
TEST(ProxyLifetimeTests, TestNullAssignment_FromNullptr_ToValue) {
278301
utils::LifetimeTracker tracker;
279302
std::vector<utils::LifetimeOperation> expected_ops;
@@ -358,6 +381,30 @@ TEST(ProxyLifetimeTests, TestPolyAssignment_ToValue_Exception) {
358381
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
359382
}
360383

384+
TEST(ProxyLifetimeTests, TestPolyAssignment_ToValue_DestructionException) {
385+
utils::LifetimeTracker tracker;
386+
std::vector<utils::LifetimeOperation> expected_ops;
387+
{
388+
pro::proxy<detail::TestThrowingDestructionFacade> p{
389+
std::in_place_type<utils::LifetimeTracker::ThrowingDestructionSession>,
390+
&tracker};
391+
expected_ops.emplace_back(1,
392+
utils::LifetimeOperationType::kValueConstruction);
393+
utils::LifetimeTracker::Session session{&tracker};
394+
expected_ops.emplace_back(2,
395+
utils::LifetimeOperationType::kValueConstruction);
396+
ASSERT_THROW(p = session, utils::DestructionFailure);
397+
ASSERT_FALSE(p.has_value());
398+
expected_ops.emplace_back(3,
399+
utils::LifetimeOperationType::kCopyConstruction);
400+
expected_ops.emplace_back(1, utils::LifetimeOperationType::kDestruction);
401+
expected_ops.emplace_back(3, utils::LifetimeOperationType::kDestruction);
402+
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
403+
}
404+
expected_ops.emplace_back(2, utils::LifetimeOperationType::kDestruction);
405+
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
406+
}
407+
361408
TEST(ProxyLifetimeTests, TestPolyAssignment_FromValue_ToNull) {
362409
utils::LifetimeTracker tracker;
363410
std::vector<utils::LifetimeOperation> expected_ops;
@@ -630,6 +677,34 @@ TEST(ProxyLifetimeTests, TestCopyAssignment_FromValue_ToValue_Exception) {
630677
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
631678
}
632679

680+
TEST(ProxyLifetimeTests,
681+
TestCopyAssignment_FromValue_ToValue_DestructionException) {
682+
utils::LifetimeTracker tracker;
683+
std::vector<utils::LifetimeOperation> expected_ops;
684+
{
685+
pro::proxy<detail::TestThrowingDestructionFacade> p1{
686+
std::in_place_type<utils::LifetimeTracker::ThrowingDestructionSession>,
687+
&tracker};
688+
expected_ops.emplace_back(1,
689+
utils::LifetimeOperationType::kValueConstruction);
690+
pro::proxy<detail::TestThrowingDestructionFacade> p2{
691+
std::in_place_type<utils::LifetimeTracker::Session>, &tracker};
692+
expected_ops.emplace_back(2,
693+
utils::LifetimeOperationType::kValueConstruction);
694+
ASSERT_THROW(p1 = p2, utils::DestructionFailure);
695+
ASSERT_FALSE(p1.has_value());
696+
ASSERT_TRUE(p2.has_value());
697+
ASSERT_EQ(ToString(*p2), "Session 2");
698+
expected_ops.emplace_back(3,
699+
utils::LifetimeOperationType::kCopyConstruction);
700+
expected_ops.emplace_back(1, utils::LifetimeOperationType::kDestruction);
701+
expected_ops.emplace_back(3, utils::LifetimeOperationType::kDestruction);
702+
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
703+
}
704+
expected_ops.emplace_back(2, utils::LifetimeOperationType::kDestruction);
705+
ASSERT_TRUE(tracker.GetOperations() == expected_ops);
706+
}
707+
633708
TEST(ProxyLifetimeTests, TestCopyAssignment_FromValue_ToSelf) {
634709
utils::LifetimeTracker tracker;
635710
std::vector<utils::LifetimeOperation> expected_ops;

‎tests/utils.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#ifndef _MSFT_PROXY_TEST_UTILS_
66
#define _MSFT_PROXY_TEST_UTILS_
77

8+
#include <exception>
89
#include <proxy/proxy.h>
910
#include <string>
1011
#include <vector>
@@ -38,6 +39,8 @@ struct ConstructionFailure : std::exception {
3839
LifetimeOperationType type_;
3940
};
4041

42+
struct DestructionFailure : std::exception {};
43+
4144
class LifetimeTracker {
4245
public:
4346
LifetimeTracker() = default;
@@ -74,6 +77,16 @@ class LifetimeTracker {
7477
LifetimeTracker* const host_;
7578
};
7679

80+
class ThrowingDestructionSession : public Session {
81+
public:
82+
using Session::Session;
83+
~ThrowingDestructionSession() noexcept(false) {
84+
if (std::uncaught_exceptions() == 0) {
85+
throw DestructionFailure{};
86+
}
87+
}
88+
};
89+
7790
const std::vector<LifetimeOperation>& GetOperations() const { return ops_; }
7891
void ThrowOnNextConstruction() { throw_on_next_construction_ = true; }
7992

0 commit comments

Comments
 (0)