Fix destructor race in LeftRight and add additional test cases

This commit is contained in:
Sebastian Messmer 2019-02-02 13:00:53 -08:00
parent d3ba6d1c39
commit 95203356cb
2 changed files with 54 additions and 20 deletions

View File

@ -31,14 +31,6 @@ private:
template <class T> template <class T>
class LeftRight final { class LeftRight final {
public: public:
LeftRight()
: _writeMutex()
, _foregroundCounterIndex{0}
, _foregroundDataIndex{0}
, _counters{{{0}, {0}}}
, _data{{{}, {}}}
, _inDestruction(false) {}
~LeftRight() { ~LeftRight() {
// from now on, no new readers/writers will be accepted (see asserts in read()/write()) // from now on, no new readers/writers will be accepted (see asserts in read()/write())
_inDestruction = true; _inDestruction = true;
@ -56,22 +48,25 @@ public:
template <typename F> template <typename F>
auto read(F&& readFunc) const { auto read(F&& readFunc) const {
detail::IncrementRAII _increment_counter(&_counters[_foregroundCounterIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index)
if(_inDestruction.load()) { if(_inDestruction.load()) {
throw std::logic_error("Issued LeftRight::read() after the destructor started running"); throw std::logic_error("Issued LeftRight::read() after the destructor started running");
} }
detail::IncrementRAII _increment_counter(&_counters[_foregroundCounterIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index)
return readFunc(_data[_foregroundDataIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index) return readFunc(_data[_foregroundDataIndex.load()]); // NOLINT(cppcoreguidelines-pro-bounds-constant-array-index)
} }
// Throwing from write would result in invalid state // Throwing an exception in writeFunc is ok but causes the state to be either the old or the new state,
// depending on if the first or the second call to writeFunc threw.
template <typename F> template <typename F>
auto write(F&& writeFunc) { auto write(F&& writeFunc) {
std::unique_lock<std::mutex> lock(_writeMutex);
if(_inDestruction.load()) { if(_inDestruction.load()) {
throw std::logic_error("Issued LeftRight::read() after the destructor started running"); throw std::logic_error("Issued LeftRight::read() after the destructor started running");
} }
std::unique_lock<std::mutex> lock(_writeMutex);
return _write(writeFunc); return _write(writeFunc);
} }
@ -133,7 +128,7 @@ private:
_waitForBackgroundCounterToBeZero(localCounterIndex); _waitForBackgroundCounterToBeZero(localCounterIndex);
// 6. Write to B // 6. Write to B
_callWriteFuncOnBackgroundInstance(writeFunc, localDataIndex); return _callWriteFuncOnBackgroundInstance(writeFunc, localDataIndex);
} }
template<class F> template<class F>
@ -155,11 +150,11 @@ private:
} }
std::mutex _writeMutex; std::mutex _writeMutex;
std::atomic<uint8_t> _foregroundCounterIndex; std::atomic<uint8_t> _foregroundCounterIndex = {0};
std::atomic<uint8_t> _foregroundDataIndex; std::atomic<uint8_t> _foregroundDataIndex = {0};
mutable std::array<std::atomic<int32_t>, 2> _counters; mutable std::array<std::atomic<int32_t>, 2> _counters = {{{0}, {0}}};
std::array<T, 2> _data; std::array<T, 2> _data = {{{}, {}}};
std::atomic<bool> _inDestruction; std::atomic<bool> _inDestruction = {false};
}; };
} }

View File

@ -30,6 +30,14 @@ TEST(LeftRightTest, givenVector_whenWritingAndReading_thenChangesArePresent) {
EXPECT_EQ((vector<int>{5, 6}), read); EXPECT_EQ((vector<int>{5, 6}), read);
} }
TEST(LeftRightTest, givenVector_whenWritingReturnsValue_thenValueIsReturned) {
LeftRight<vector<int>> obj;
auto a = obj.write([] (auto&) -> int {return 5;});
static_assert(std::is_same<int, decltype(a)>::value, "");
EXPECT_EQ(5, a);
}
TEST(LeftRightTest, readsCanBeConcurrent) { TEST(LeftRightTest, readsCanBeConcurrent) {
LeftRight<int> obj; LeftRight<int> obj;
std::atomic<int> num_running_readers{0}; std::atomic<int> num_running_readers{0};
@ -157,7 +165,7 @@ TEST(LeftRightTest, whenWriteThrowsException_thenThrowsThrough) {
); );
} }
TEST(LeftRightTest, givenInt_whenWriteThrowsException_thenResetsToOldState) { TEST(LeftRightTest, givenInt_whenWriteThrowsExceptionOnFirstCall_thenResetsToOldState) {
LeftRight<int> obj; LeftRight<int> obj;
obj.write([](auto& obj) {obj = 5;}); obj.write([](auto& obj) {obj = 5;});
@ -180,6 +188,37 @@ TEST(LeftRightTest, givenInt_whenWriteThrowsException_thenResetsToOldState) {
EXPECT_EQ(5, read); EXPECT_EQ(5, read);
} }
// note: each write is executed twice, on the foreground and background copy.
// We need to test a thrown exception in either call is handled correctly.
TEST(LeftRightTest, givenInt_whenWriteThrowsExceptionOnSecondCall_thenKeepsNewState) {
LeftRight<int> obj;
obj.write([](auto& obj) {obj = 5;});
bool write_called = false;
EXPECT_THROW(
obj.write([&](auto& obj) {
obj = 6;
if (write_called) {
// this is the second time the write callback is executed
throw MyException();
} else {
write_called = true;
}
}),
MyException
);
// check reading it returns new value
int read = obj.read([] (auto& obj) {return obj;});
EXPECT_EQ(6, read);
// check changes are also present in background copy
obj.write([] (auto&) {}); // this switches to the background copy
read = obj.read([] (auto& obj) {return obj;});
EXPECT_EQ(6, read);
}
TEST(LeftRightTest, givenVector_whenWriteThrowsException_thenResetsToOldState) { TEST(LeftRightTest, givenVector_whenWriteThrowsException_thenResetsToOldState) {
LeftRight<vector<int>> obj; LeftRight<vector<int>> obj;