diff --git a/doc/website/release-notes/iceoryx-unreleased.md b/doc/website/release-notes/iceoryx-unreleased.md index c37f60d2bb..b82c0a936a 100644 --- a/doc/website/release-notes/iceoryx-unreleased.md +++ b/doc/website/release-notes/iceoryx-unreleased.md @@ -137,6 +137,8 @@ - Listener examples need to take all samples in the callback [#2251](https://github.com/eclipse-iceoryx/iceoryx/issues/2251) - 'iox::string' tests can exceed the translation unit compilation timeout [#2278](https://github.com/eclipse-iceoryx/iceoryx/issues/2278) - Building iceoryx with bazel on Windows is broken [#2320](https://github.com/eclipse-iceoryx/iceoryx/issues/2320) +- Fix wrong memory orders in SpscSoFi [#2177](https://github.com/eclipse-iceoryx/iceoryx/issues/2177) +- **Refactoring:** diff --git a/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.hpp b/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.hpp index 7b60a8b1c5..e104c44046 100644 --- a/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.hpp +++ b/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.hpp @@ -25,117 +25,118 @@ #include #include +#include namespace iox { namespace concurrent { -/// @brief -/// Thread safe producer and consumer queue with a safe overflowing behavior. -/// SpscSofi is designed in a FIFO Manner but prevents data loss when pushing into -/// a full SpscSofi. When SpscSofi is full and a Sender tries to push, the data at the -/// current read position will be returned. SpscSofi is threadsafe without using -/// locks. When the buffer is filled, new data is written starting at the -/// beginning of the buffer and overwriting the old.The SpscSofi is especially -/// designed to provide fixed capacity storage. When its capacity is exhausted, -/// newly inserted elements will cause elements either at the beginning -/// to be overwritten.The SpscSofi only allocates memory when -/// created , capacity can be is adjusted explicitly. -/// +/// @brief Thread safe lock-free single producer and single consumer queue with a safe +/// overflowing behavior +/// @note When SpscSoFi is full and a sender tries to push, the data at the current read pos will be +/// returned. This behavior mimics a FiFo queue but prevents resource leaks when pushing into +/// a full SpscSoFi. +/// SpscSoFi is especially designed to provide fixed capacity storage. +/// It's an expected behavior that when push/pop are called concurrently and SpscSoFi is full, as +/// many elements as specified with 'CapacityValue' can be removed /// @param[in] ValueType DataType to be stored, must be trivially copyable /// @param[in] CapacityValue Capacity of the SpscSofi template class SpscSofi { static_assert(std::is_trivially_copyable::value, - "SpscSofi can handle only trivially copyable data types"); + "SpscSofi can only handle trivially copyable data types since 'memcpy' is used internally"); /// @brief Check if Atomic integer is lockfree on platform /// ATOMIC_INT_LOCK_FREE = 2 - is always lockfree /// ATOMIC_INT_LOCK_FREE = 1 - is sometimes lockfree /// ATOMIC_INT_LOCK_FREE = 0 - is never lockfree static_assert(2 <= ATOMIC_INT_LOCK_FREE, "SpscSofi is not able to run lock free on this data type"); - /// @brief Internal size needs to be bigger than the size desirred by the user - /// This is because of buffer empty detection and overflow handling - static constexpr uint32_t INTERNAL_SIZE_ADD_ON = 1; - - /// @brief This is the resulting internal size on creation - static constexpr uint32_t INTERNAL_SPSC_SOFI_SIZE = CapacityValue + INTERNAL_SIZE_ADD_ON; + // To ensure a consumer gets at least the amount of capacity of data when a queue is full, an additional free + // slot (add-on) is required. + // ======================================================================== + // Consider the following scenario when there is no "capacity add-on": + // 1. CapacityValue = 2 + // |--A--|--B--| + // ^ + // w=2, r=0 + // 2. The producer thread pushes a new element + // 3. Increment the read position (this effectively reduces the capacity and is the reason the internal capacity + // needs to be larger; + // |--A--|--B--| + // ^ ^ + // w=2 r=1 + // 4. The producer thread is suspended, the consumer thread pops a value + // |--A--|-----| + // ^ + // w=2, r=2 + // 5. The consumer tries to pop another value but the queue looks empty as + // write position == read position: the consumer cannot pop + // out CAPACITY amount of samples even though the queue was full + // ======================================================================== + // With "capacity add-on" + // 1. CapacityValue = 2, InternalCapacity = 3 + // |--A--|--B--|----| + // ^ ^ + // r=0 w=2 + // 2. The producer threads pushes a new element + // 3. First write the element at index 2 % capacity and increment the write index + // |--A--|--B--|--C--| + // ^ + // w=3, r=0, + // 4. Then increment the read position and return the overflowing 'A' + // |-----|--B--|--C--| + // ^ ^ + // w=3 r=1 + // 5. The producer thread is suspended, the consumer thread pops a value + // |--A--|-----|--C--| + // ^ ^ + // w=3 r=2 + // 6. The consumer thread pops another value + // |--A--|-----|-----| + // ^ + // w=3, r=3 + // 7. Now, write position == read position so we cannot pop another element: the queue looks empty. We managed to + // pop CapacityValue elements + // ======================================================================== + static constexpr uint32_t INTERNAL_CAPACITY_ADDON = 1; + + /// @brief Internal capacity of the queue at creation + static constexpr uint32_t INTERNAL_SPSC_SOFI_CAPACITY = CapacityValue + INTERNAL_CAPACITY_ADDON; public: - /// @brief default constructor which constructs an empty sofi + /// @brief default constructor which constructs an empty SpscSofi SpscSofi() noexcept = default; - /// @brief pushs an element into SpscSofi. if SpscSofi is full the oldest data will be + /// @brief push an element into SpscSofi. if SpscSofi is full the oldest data will be /// returned and the pushed element is stored in its place instead. - /// @param[in] valueIn value which should be stored - /// @param[out] valueOut if SpscSofi is overflowing the value of the overridden value + /// @param[in] value_in value which should be stored + /// @param[out] value_out if SpscSofi is overflowing the value of the overridden value /// is stored here - /// @concurrent restricted thread safe: single pop, single push no - /// push calls from multiple contexts - /// @return return true if push was sucessfull else false. - /// @code - /// (initial situation, SpscSofi is FULL) - /// Start|-----A-------| - /// |-----B-------| - /// |-----C-------| - /// |-----D-------| - /// - /// - /// (calling push with data ’E’) - /// Start|-----E-------| - /// |-----A-------| - /// |-----B-------| - /// |-----C-------| - /// (’D’ is returned as valueOut) - /// - /// ################################################################### - /// - /// (if SpscSofi is not FULL , calling push() add new data) - /// Start|-------------| - /// |-------------| ( Initial SpscSofi ) - /// (push() Called two times) - /// - /// |-------------| - /// (New Data) - /// |-------------| - /// (New Data) - /// @endcode + /// @note restricted thread safe: can only be called from one thread. The authorization to push into the + /// SpscSofi can be transferred to another thread if appropriate synchronization mechanisms are used. + /// @return return true if push was successful else false. + /// @remarks + /// 1. SpscSofi is empty |-----|-----| + /// 2. push an element |--A--|-----| + /// 3. push an element |--A--|--B--| + /// 5. SpscSofi is full + /// 6. push an element |--C--|--B--| -> value_out is set to 'A' bool push(const ValueType& valueIn, ValueType& valueOut) noexcept; /// @brief pop the oldest element /// @param[out] valueOut storage of the pop'ed value - /// @concurrent restricted thread safe: single pop, single push no - /// pop or popIf calls from multiple contexts + /// @concurrent restricted thread safe: can only be called from one thread. The authorization to pop from the + /// SpscSofi can be transferred to another thread if appropriate synchronization mechanisms are used. /// @return false if SpscSofi is empty, otherwise true bool pop(ValueType& valueOut) noexcept; - /// @brief conditional pop call to provide an alternative for a peek - /// and pop approach. If the verificator returns true the - /// peeked element is returned. - /// @param[out] valueOut storage of the pop'ed value - /// @param[in] verificator callable of type bool(const ValueType& peekValue) - /// which takes the value which would be pop'ed as argument and returns - /// true if it should be pop'ed, otherwise false - /// @code - /// int limit = 7128; - /// mysofi.popIf(value, [=](const ValueType & peek) - /// { - /// return peek < limit; // pop only when peek is smaller than limit - /// } - /// ); // pop's a value only if it is smaller than 9012 - /// @endcode - /// @concurrent restricted thread safe: single pop, single push no - /// pop or popIf calls from multiple contexts - /// @return false if SpscSofi is empty or when verificator returns false, otherwise true - template - bool popIf(ValueType& valueOut, const Verificator_T& verificator) noexcept; - /// @brief returns true if SpscSofi is empty, otherwise false /// @note the use of this function is limited in the concurrency case. if you /// call this and in another thread pop is called the result can be out /// of date as soon as you require it - /// @concurrent unrestricted thread safe + /// @concurrent unrestricted thread safe (the result might already be outdated when used). Expected to be called + /// from either the producer or the consumer thread but not from a third thread bool empty() const noexcept; /// @brief resizes SpscSofi @@ -150,15 +151,16 @@ class SpscSofi uint64_t capacity() const noexcept; /// @brief returns the current size of SpscSofi - /// @concurrent unrestricted thread safe + /// @concurrent unrestricted thread safe (the result might already be outdated when used). Expected to be called + /// from either the producer or the consumer thread but not from a third thread uint64_t size() const noexcept; private: - UninitializedArray m_data; - uint64_t m_size = INTERNAL_SPSC_SOFI_SIZE; + std::pair getReadWritePositions() const noexcept; - /// @brief the write/read pointers are "atomic pointers" so that they are not - /// reordered (read or written too late) + private: + UninitializedArray m_data; + uint64_t m_size = INTERNAL_SPSC_SOFI_CAPACITY; Atomic m_readPosition{0}; Atomic m_writePosition{0}; }; diff --git a/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.inl b/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.inl index 146ac726dd..3d935d2362 100644 --- a/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.inl +++ b/iceoryx_hoofs/concurrent/buffer/include/iox/detail/spsc_sofi.inl @@ -27,11 +27,11 @@ namespace concurrent template inline uint64_t SpscSofi::capacity() const noexcept { - return m_size - INTERNAL_SIZE_ADD_ON; + return m_size - INTERNAL_CAPACITY_ADDON; } template -inline uint64_t SpscSofi::size() const noexcept +inline std::pair SpscSofi::getReadWritePositions() const noexcept { uint64_t readPosition{0}; uint64_t writePosition{0}; @@ -39,17 +39,44 @@ inline uint64_t SpscSofi::size() const noexcept { readPosition = m_readPosition.load(std::memory_order_relaxed); writePosition = m_writePosition.load(std::memory_order_relaxed); + + // The while loop is needed to avoid the following scenarios: + // 1. Implementation to get the size: size = m_writePosition - m_readPosition; + // - consumer reads m_writePosition + // - consumer thread gets suspended + // - producer pushes 100 times + // - consumer reads m_readPosition + // => m_readPosition will be past m_writePosition and one would get a negative size (or the positive unsigned + // equivalent) + // 2. Implementation to get the size: readPosition = m_readPosition; size = m_writePosition - readPosition; + // - consumer stores m_readPosition in readPosition + // - consumer thread gets suspended + // - producer pushes 100 times + // - consumer reads m_writePosition + // => m_writePosition will be past readPosition + Capacity and one would get a size which is much larger than + // the capacity + // =========================================== + // Note: it is still possible to return a size that is not up-to-date anymore but at least + // the returned size is logically valid } while (m_writePosition.load(std::memory_order_relaxed) != writePosition || m_readPosition.load(std::memory_order_relaxed) != readPosition); + return {readPosition, writePosition}; +} + + +template +inline uint64_t SpscSofi::size() const noexcept +{ + auto [readPosition, writePosition] = getReadWritePositions(); return writePosition - readPosition; } template inline bool SpscSofi::setCapacity(const uint64_t newSize) noexcept { - uint64_t newInternalSize = newSize + INTERNAL_SIZE_ADD_ON; - if (empty() && (newInternalSize <= INTERNAL_SPSC_SOFI_SIZE)) + uint64_t newInternalSize = newSize + INTERNAL_CAPACITY_ADDON; + if (empty() && (newInternalSize <= INTERNAL_SPSC_SOFI_CAPACITY)) { m_size = newInternalSize; @@ -65,74 +92,62 @@ inline bool SpscSofi::setCapacity(const uint64_t newSi template inline bool SpscSofi::empty() const noexcept { - uint64_t currentReadPosition{0}; - bool isEmpty{false}; - - do - { - /// @todo iox-#1695 read before write since the writer increments the aba counter!!! - /// @todo iox-#1695 write doc with example!!! - currentReadPosition = m_readPosition.load(std::memory_order_acquire); - uint64_t currentWritePosition = m_writePosition.load(std::memory_order_acquire); - - isEmpty = (currentWritePosition == currentReadPosition); - // we need compare without exchange - } while (!(currentReadPosition == m_readPosition.load(std::memory_order_acquire))); - - return isEmpty; + auto [readPosition, writePosition] = getReadWritePositions(); + return readPosition == writePosition; } template inline bool SpscSofi::pop(ValueType& valueOut) noexcept { - return popIf(valueOut, [](ValueType) { return true; }); -} - -template -template -inline bool SpscSofi::popIf(ValueType& valueOut, const Verificator_T& verificator) noexcept -{ - uint64_t currentReadPosition = m_readPosition.load(std::memory_order_acquire); uint64_t nextReadPosition{0}; - bool popWasSuccessful{true}; + // Memory order relaxed is enough since: + // - no synchronization needed for m_readPosition + // - if m_writePosition is loaded before m_readPosition and m_readPosition changed, it will be detected by the + // compare_exchange loop + uint64_t currentReadPosition = m_readPosition.load(std::memory_order_relaxed); + do { + // SYNC POINT READ: m_data + // See explanation of the corresponding synchronization point in push() if (currentReadPosition == m_writePosition.load(std::memory_order_acquire)) { nextReadPosition = currentReadPosition; popWasSuccessful = false; + // We cannot just return false (i.e. we need to continue the loop) to avoid the following situation: + // 0. Initial situation (the queue is full) + // |----|--B--|--C--| + // ^ ^ + // w=3 r=1 + // 1. The consumer thread loads m_writePosition => 3 + // |----|--B--|--C--| + // ^ ^ + // w=3 r=1 + // 2. The producer thread pushes two times + // |--D--|--E--|-----| + // ^ ^ + // r=3 w=5 + // 3. The consumer thread loads m_readPosition => 3 The pop method returns false + // => Whereas the queue was full, pop returned false giving the impression that the queue if empty } else { - // we use memcpy here, since the copy assignment is not thread safe in general (we might have an overflow in - // the push thread and invalidates the object while the copy is running and therefore works on an - // invalid object); memcpy is also not thread safe, but we discard the object anyway and read it - // again if its overwritten in between; this is only relevant for types larger than pointer size - // assign the user data + // we use memcpy here, to ensure that there is no logic in copying the data std::memcpy(&valueOut, &m_data[currentReadPosition % m_size], sizeof(ValueType)); + nextReadPosition = currentReadPosition + 1U; + popWasSuccessful = true; - /// @brief first we need to peak valueOut if it is fitting the condition and then we have to verify - /// if valueOut is not am invalid object, this could be the case if the read position has - /// changed - if (m_readPosition.load(std::memory_order_relaxed) == currentReadPosition && !verificator(valueOut)) - { - popWasSuccessful = false; - nextReadPosition = currentReadPosition; - } - else - { - nextReadPosition = currentReadPosition + 1U; - popWasSuccessful = true; - } + // We need to check if m_readPosition hasn't changed otherwise valueOut might be corrupted + // ============================================= + // While memory synchronization is not needed for m_readPosition, we need to ensure that the + // 'memcpy' happens before updating m_readPosition. + // Corresponding m_readPosition load/acquire is in the CAS loop of push method + // ============================================= + // ABA problem: m_readPosition is an uint64_t. Assuming a thread is pushing at a rate of 1 GHz + // while this thread is blocked, we would still need more than 500 years to overflow + // m_readPosition and encounter the ABA problem } - - // compare and swap - // if(m_readPosition == currentReadPosition) - // m_readPosition = l_next_aba_read_pos - // else - // currentReadPosition = m_readPosition - // Assign m_aba_read_p to next readable location } while (!m_readPosition.compare_exchange_weak( currentReadPosition, nextReadPosition, std::memory_order_acq_rel, std::memory_order_acquire)); @@ -144,42 +159,94 @@ inline bool SpscSofi::push(const ValueType& valueIn, V { constexpr bool SOFI_OVERFLOW{false}; + // Memory order relaxed is enough since: + // - no synchronization needed as we are loading a value only modified in this method and this method cannot be + // accessed concurrently + // - the operation cannot move below without observable changes uint64_t currentWritePosition = m_writePosition.load(std::memory_order_relaxed); uint64_t nextWritePosition = currentWritePosition + 1U; m_data[currentWritePosition % m_size] = valueIn; + // SYNC POINT WRITE: m_data + // We need to make sure that writing the value happens before incrementing the + // m_writePosition otherwise the following scenario can happen: + // 1. m_writePosition is increased (but the value has not been written yet) + // 2. The consumer thread calls pop(): we check if the queue is empty => no + // 3. In pop(), when we read a value a data race can occur + // With memory_order_release, this cannot happen as it is guaranteed that writing the data + // happens before incrementing m_writePosition + // ======================================= + // Note that the following situation can still happen (but, although it is an inherent race with + // concurrent algorithms, it is not a data race and therefore not a problem): + // 1. There is an empty queue + // 2. A push operation is in progress, the value has been written but 'm_writePosition' was not + // yet advanced + // 3. The consumer thread performs a pop operation and the check for an empty queue is true + // resulting in a failed pop + // 4. The push operation is finished by advancing m_writePos and synchronizing the memory + // 5. The consumer thread missed the chance to pop the element in the blink of an eye m_writePosition.store(nextWritePosition, std::memory_order_release); - uint64_t currentReadPosition = m_readPosition.load(std::memory_order_acquire); + // Memory order relaxed is enough since: + // - no synchronization needed when loading + // - the operation cannot move below without observable changes + uint64_t currentReadPosition = m_readPosition.load(std::memory_order_relaxed); - // check if there is a free position for the next push + // Check if queue is full: since we have an extra element (INTERNAL_CAPACITY_ADD_ON), we need to + // check if there is a free position for the *next* write position if (nextWritePosition < currentReadPosition + m_size) { return !SOFI_OVERFLOW; } - // this is an overflow situation, which means that the next push has no free position, therefore the oldest value - // needs to be passed back to the caller - - uint64_t nextReadPosition = currentReadPosition + 1U; - - // we need to update the read position - // a) it works, then we need to pass the overflow value back - // b) it doesn't work, which means that the pop thread already took the value in the meantime an no further action - // is required - // memory order success is memory_order_acq_rel - // - this is to prevent the reordering of m_writePosition.store(...) after the increment of the m_readPosition - // - in case of an overflow, this might result in the pop thread getting one element less than the capacity of - // the SoFi if the push thread is suspended in between this two statements - // - it's still possible to get more elements than the capacity, but this is an inherent issue with concurrent - // queues and cannot be prevented since there can always be a push during a pop operation - // - another issue might be that two consecutive pushes (not concurrent) happen on different CPU cores without - // synchronization, then the memory also needs to be synchronized for the overflow case - // memory order failure is memory_order_relaxed since there is no further synchronization needed if there is no - // overflow + // This is an overflow situation so we will need to read the overwritten value + // however, it could be that pop() was called in the meantime, i.e. m_readPosition was increased. + // Memory order success needs to be memory_order_acq_rel to prevent the reordering of + // m_writePosition.store(...) after the increment of the m_readPosition. Otherwise, in case of + // an overflow, this might result in the pop thread getting one element less than the capacity + // of the SoFi if the push thread is suspended in between this two statements. + // It's still possible to get more elements than the capacity, but this is an inherent issue + // with concurrent queues and cannot be prevented since there can always be a push during a pop + // operation. + // Another issue might be that two consecutive pushes (not concurrent) happen on different CPU + // cores without synchronization, then the memory also needs to be synchronized for the overflow + // case. + // Memory order failure needs to be memory_order_acquire to match the corresponding m_readPosition store/release in + // the CAS loop of the pop method + // ====================================== + // ABA problem: m_readPosition is an uint64_t. Assuming a thread is popping at a rate of 1 GHz while + // this thread is blocked, we would still need more than 500 years to overflow m_readPosition and + // encounter the ABA problem if (m_readPosition.compare_exchange_strong( - currentReadPosition, nextReadPosition, std::memory_order_acq_rel, std::memory_order_relaxed)) + currentReadPosition, currentReadPosition + 1U, std::memory_order_acq_rel, std::memory_order_acquire)) { + // Since INTERNAL_SOFI_CAPACITY = CapacityValue + 1, it can happen that we return more + // elements than the CapacityValue by calling push and pop concurrently (in case of an + // overflow). This is an inherent behavior with concurrent queues. Scenario example + // (CapacityValue = 2): + // 0. Initial situation (before the call to push) + // |--A--|--B--|----| + // ^ ^ + // r=0 w=2 + // 1. Thread 1, pushes a new value and increases m_readPosition (overflow situation) + // |--A--|--B--|--C--| + // ^ ^ + // w=3, r=1 + // 2. Now, thread 1 is interrupted and another thread pops as many elements as possible + // 3. pop() -> returns B (First value returned by pop) + // |--A--|-(B)-|--C--| + // ^ ^ + // w=3 r=2 + // 4. pop() -> returns C (Second value returned by pop) + // |--A--|-(B)-|-(C)-| + // ^ + // w=3, r=3 + // 5. pop() -> nothing to return + // 6. Finally, thread 1 resumes and returns A (Third value [additional value] returned by + // push) + // |-(A)-|-(B)-|-(C)-| + // ^ + // w=3, r=3 std::memcpy(&valueOut, &m_data[currentReadPosition % m_size], sizeof(ValueType)); return SOFI_OVERFLOW; } diff --git a/iceoryx_hoofs/test/moduletests/test_concurrent_spsc_sofi.cpp b/iceoryx_hoofs/test/moduletests/test_concurrent_spsc_sofi.cpp index dd48bf5a6c..2d724fb00c 100644 --- a/iceoryx_hoofs/test/moduletests/test_concurrent_spsc_sofi.cpp +++ b/iceoryx_hoofs/test/moduletests/test_concurrent_spsc_sofi.cpp @@ -388,7 +388,7 @@ TEST_F(SpscSofiTest, ResizeAndSizeFillUp) } } -TEST_F(SpscSofiTest, PopIfWithValidCondition) +TEST_F(SpscSofiTest, Pop) { ::testing::Test::RecordProperty("TEST_ID", "f149035c-21cc-4f7d-ba4d-564a645e933b"); sofi.push(10, returnVal); @@ -396,33 +396,22 @@ TEST_F(SpscSofiTest, PopIfWithValidCondition) sofi.push(12, returnVal); int output{-1}; - bool result = sofi.popIf(output, [](const int& peek) { return peek < 20; }); + bool result = sofi.pop(output); EXPECT_EQ(result, true); EXPECT_EQ(output, 10); } -TEST_F(SpscSofiTest, PopIfWithInvalidCondition) -{ - ::testing::Test::RecordProperty("TEST_ID", "1a494c28-928f-48f4-8b01-e68dfbd7563e"); - sofi.push(15, returnVal); - sofi.push(16, returnVal); - sofi.push(17, returnVal); - - bool result = sofi.popIf(returnVal, [](const int& peek) { return peek < 5; }); - - EXPECT_EQ(result, false); -} -TEST_F(SpscSofiTest, PopIfOnEmpty) +TEST_F(SpscSofiTest, PopOnEmpty) { ::testing::Test::RecordProperty("TEST_ID", "960ad78f-cb9b-4c34-a077-6adb343a841c"); - bool result = sofi.popIf(returnVal, [](const int& peek) { return peek < 7; }); + bool result = sofi.pop(returnVal); EXPECT_EQ(result, false); } -TEST_F(SpscSofiTest, PopIfFullWithValidCondition) +TEST_F(SpscSofiTest, PopFull) { ::testing::Test::RecordProperty("TEST_ID", "167f2f01-f926-4442-bc4f-ff5e7cfe9fe0"); constexpr int INITIAL_VALUE = 100; @@ -432,42 +421,21 @@ TEST_F(SpscSofiTest, PopIfFullWithValidCondition) sofi.push(i + INITIAL_VALUE, returnVal); } - bool result = sofi.popIf(returnVal, [](const int& peek) { return peek < 150; }); + bool result = sofi.pop(returnVal); EXPECT_EQ(result, true); EXPECT_EQ(returnVal, INITIAL_VALUE + OFFSET); } -TEST_F(SpscSofiTest, PopIfFullWithInvalidCondition) -{ - ::testing::Test::RecordProperty("TEST_ID", "672881b9-eebd-471d-9d62-e792a8b8013f"); - for (int i = 0; i < static_cast(sofi.capacity()) + 2; i++) - { - sofi.push(i + 100, returnVal); - } - - bool result = sofi.popIf(returnVal, [](const int& peek) { return peek < 50; }); - - EXPECT_EQ(result, false); -} - -TEST_F(SpscSofiTest, PopIfValidEmptyAfter) +TEST_F(SpscSofiTest, PopEmptyAfter) { ::testing::Test::RecordProperty("TEST_ID", "19444dcd-7746-4e6b-a3b3-398c9d62317d"); + sofi.push(2, returnVal); - sofi.popIf(returnVal, [](const int& peek) { return peek < 50; }); + sofi.pop(returnVal); EXPECT_EQ(sofi.empty(), true); } -TEST_F(SpscSofiTest, PopIfInvalidNotEmptyAfter) -{ - ::testing::Test::RecordProperty("TEST_ID", "cadd7f02-6fe5-49a5-bd5d-837f5fcb2a71"); - sofi.push(200, returnVal); - - sofi.popIf(returnVal, [](const int& peek) { return peek < 50; }); - - EXPECT_EQ(sofi.empty(), false); -} } // namespace