Copilot commented on code in PR #13999: URL: https://github.com/apache/trafficserver/pull/13999#discussion_r4473845006
########## 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: Each iteration destroys its only strong lease, while the thread-local cache retains only a weak pointer. Consequently, every iteration takes the slow path and this concurrent replacement test never exercises reuse or invalidation of a live cached lease. Keep `v` alive across iterations and assign subsequent loads to it so the test covers the optimized path during replacement. ########## 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: The writer can finish all 2,000 stores and set `stop` before any reader enters its loop. In that valid schedule, every reader exits immediately and `CHECK(reads.load() > 0)` fails even when the implementation is correct. Please wait for reader progress before starting the replacement loop, with bounded waiting and thread cleanup on failure, as in `test_AtomicSharedPtr.cc:90–112,143–148`. -- 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]
