cmcfarlen commented on code in PR #13999: URL: https://github.com/apache/trafficserver/pull/13999#discussion_r4474241021
########## src/tsutil/unit_tests/test_ThreadLeasedSharedPtr.cc: ########## @@ -0,0 +1,164 @@ +/** @file + + Unit tests for ThreadLeasedSharedPtr. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <catch2/catch_test_macros.hpp> + +#include "tsutil/ThreadLeasedSharedPtr.h" + +#include <atomic> +#include <memory> +#include <thread> +#include <vector> + +namespace +{ +// Records its destruction so tests can check when the shared value is released. +struct Tracked { + Tracked(int v, std::atomic<int> &destroyed) : value(v), destroyed_count(destroyed) {} + ~Tracked() { destroyed_count.fetch_add(1); } + + int value; + std::atomic<int> &destroyed_count; +}; +} // namespace + +TEST_CASE("ThreadLeasedSharedPtr shares one reference per thread", "[tsutil][ThreadLeasedSharedPtr]") +{ + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + auto original = std::make_shared<Tracked>(1, destroyed); + ptr.store(original); + long const baseline = original.use_count(); + + std::vector<std::shared_ptr<Tracked>> copies; + for (int i = 0; i < 10; ++i) { + copies.push_back(ptr.load_leased()); + } + + CHECK(copies.front().get() == original.get()); + // All ten copies count against the thread's control block, which holds one shared reference. + CHECK(original.use_count() == baseline + 1); + CHECK(copies.front().use_count() == 10); + + SECTION("the thread's reference is dropped with its last copy") + { + copies.clear(); + CHECK(original.use_count() == baseline); + } + + SECTION("a copy released on another thread still releases the shared reference") + { + auto moved = std::move(copies); + std::thread([&moved] { moved.clear(); }).join(); + CHECK(original.use_count() == baseline); + } +} + +TEST_CASE("ThreadLeasedSharedPtr follows store and exchange", "[tsutil][ThreadLeasedSharedPtr]") +{ + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + + ptr.store(std::make_shared<Tracked>(1, destroyed)); + auto first = ptr.load_leased(); + REQUIRE(first); + CHECK(first->value == 1); + + ptr.store(std::make_shared<Tracked>(2, destroyed)); + auto second = ptr.load_leased(); + REQUIRE(second); + CHECK(second->value == 2); + // A reference taken before the store still holds the old value. + CHECK(first->value == 1); + CHECK(destroyed.load() == 0); + + first.reset(); + CHECK(destroyed.load() == 1); + + auto previous = ptr.exchange(nullptr); + CHECK(previous.get() == second.get()); + CHECK(ptr.load_leased() == nullptr); + + previous.reset(); + second.reset(); + CHECK(destroyed.load() == 2); +} + +TEST_CASE("ThreadLeasedSharedPtr does not extend the value's lifetime when idle", "[tsutil][ThreadLeasedSharedPtr]") +{ + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + + ptr.store(std::make_shared<Tracked>(1, destroyed)); + ptr.load_leased().reset(); + std::thread([&ptr] { ptr.load_leased().reset(); }).join(); + + // With no references outstanding, dropping the stored value destroys it immediately. + ptr.store(nullptr); + CHECK(destroyed.load() == 1); +} + +TEST_CASE("ThreadLeasedSharedPtr readers see whole values during replacement", "[tsutil][ThreadLeasedSharedPtr]") +{ + constexpr int READERS = 4; + constexpr int STORES = 2000; + + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + ptr.store(std::make_shared<Tracked>(0, destroyed)); + + std::atomic<bool> stop{false}; + std::atomic<int> bad_reads{0}; + std::atomic<int> reads{0}; + + std::vector<std::thread> readers; + for (int i = 0; i < READERS; ++i) { + readers.emplace_back([&] { + int last = 0; + while (!stop.load(std::memory_order_relaxed)) { + auto v = ptr.load_leased(); Review Comment: Good catch. Readers now keep the previous value alive across loads, so unchanged generations take the cached-lease path while the writer replaces values. Fixed in ea00d1cab5. ########## src/tsutil/unit_tests/test_ThreadLeasedSharedPtr.cc: ########## @@ -0,0 +1,164 @@ +/** @file + + Unit tests for ThreadLeasedSharedPtr. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include <catch2/catch_test_macros.hpp> + +#include "tsutil/ThreadLeasedSharedPtr.h" + +#include <atomic> +#include <memory> +#include <thread> +#include <vector> + +namespace +{ +// Records its destruction so tests can check when the shared value is released. +struct Tracked { + Tracked(int v, std::atomic<int> &destroyed) : value(v), destroyed_count(destroyed) {} + ~Tracked() { destroyed_count.fetch_add(1); } + + int value; + std::atomic<int> &destroyed_count; +}; +} // namespace + +TEST_CASE("ThreadLeasedSharedPtr shares one reference per thread", "[tsutil][ThreadLeasedSharedPtr]") +{ + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + auto original = std::make_shared<Tracked>(1, destroyed); + ptr.store(original); + long const baseline = original.use_count(); + + std::vector<std::shared_ptr<Tracked>> copies; + for (int i = 0; i < 10; ++i) { + copies.push_back(ptr.load_leased()); + } + + CHECK(copies.front().get() == original.get()); + // All ten copies count against the thread's control block, which holds one shared reference. + CHECK(original.use_count() == baseline + 1); + CHECK(copies.front().use_count() == 10); + + SECTION("the thread's reference is dropped with its last copy") + { + copies.clear(); + CHECK(original.use_count() == baseline); + } + + SECTION("a copy released on another thread still releases the shared reference") + { + auto moved = std::move(copies); + std::thread([&moved] { moved.clear(); }).join(); + CHECK(original.use_count() == baseline); + } +} + +TEST_CASE("ThreadLeasedSharedPtr follows store and exchange", "[tsutil][ThreadLeasedSharedPtr]") +{ + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + + ptr.store(std::make_shared<Tracked>(1, destroyed)); + auto first = ptr.load_leased(); + REQUIRE(first); + CHECK(first->value == 1); + + ptr.store(std::make_shared<Tracked>(2, destroyed)); + auto second = ptr.load_leased(); + REQUIRE(second); + CHECK(second->value == 2); + // A reference taken before the store still holds the old value. + CHECK(first->value == 1); + CHECK(destroyed.load() == 0); + + first.reset(); + CHECK(destroyed.load() == 1); + + auto previous = ptr.exchange(nullptr); + CHECK(previous.get() == second.get()); + CHECK(ptr.load_leased() == nullptr); + + previous.reset(); + second.reset(); + CHECK(destroyed.load() == 2); +} + +TEST_CASE("ThreadLeasedSharedPtr does not extend the value's lifetime when idle", "[tsutil][ThreadLeasedSharedPtr]") +{ + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + + ptr.store(std::make_shared<Tracked>(1, destroyed)); + ptr.load_leased().reset(); + std::thread([&ptr] { ptr.load_leased().reset(); }).join(); + + // With no references outstanding, dropping the stored value destroys it immediately. + ptr.store(nullptr); + CHECK(destroyed.load() == 1); +} + +TEST_CASE("ThreadLeasedSharedPtr readers see whole values during replacement", "[tsutil][ThreadLeasedSharedPtr]") +{ + constexpr int READERS = 4; + constexpr int STORES = 2000; + + std::atomic<int> destroyed{0}; + ThreadLeasedSharedPtr<Tracked> ptr; + ptr.store(std::make_shared<Tracked>(0, destroyed)); + + std::atomic<bool> stop{false}; + std::atomic<int> bad_reads{0}; + std::atomic<int> reads{0}; + + std::vector<std::thread> readers; + for (int i = 0; i < READERS; ++i) { + readers.emplace_back([&] { + int last = 0; + while (!stop.load(std::memory_order_relaxed)) { + auto v = ptr.load_leased(); + // Values are stored in increasing order, so a reader must never see one go backwards. + if (!v || v->value < last) { + bad_reads.fetch_add(1); + } else { + last = v->value; + } + reads.fetch_add(1, std::memory_order_relaxed); + } + }); + } + + for (int i = 1; i <= STORES; ++i) { + ptr.store(std::make_shared<Tracked>(i, destroyed)); + } + stop.store(true); Review Comment: Agreed. The writer now waits (bounded, 5 s) until every reader has done its first load before it starts storing, and it stops and joins the readers before failing if they don't start. Fixed in ea00d1cab5. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
