From 0bcfe2ade90d5c33a5c9a6d8912d820e23722177 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Tue, 16 Jun 2026 11:03:56 -0500 Subject: [PATCH 1/2] Add unit tests for `Action` logic Authored with Claude Sonnet and Claude Opus. --- src/iocore/eventsystem/CMakeLists.txt | 4 + .../eventsystem/unit_tests/test_Action.cc | 254 ++++++++++++++++++ 2 files changed, 258 insertions(+) create mode 100644 src/iocore/eventsystem/unit_tests/test_Action.cc diff --git a/src/iocore/eventsystem/CMakeLists.txt b/src/iocore/eventsystem/CMakeLists.txt index 4d4a8c50c52..a6f4d6fc436 100644 --- a/src/iocore/eventsystem/CMakeLists.txt +++ b/src/iocore/eventsystem/CMakeLists.txt @@ -68,6 +68,10 @@ if(BUILD_TESTING) target_link_libraries(test_Continuation ts::inkevent configmanager Catch2::Catch2WithMain) add_catch2_test(NAME test_Continuation COMMAND test_Continuation) + add_executable(test_Action unit_tests/test_Action.cc) + target_link_libraries(test_Action ts::inkevent configmanager Catch2::Catch2WithMain) + add_catch2_test(NAME test_Action COMMAND test_Action) + endif() clang_tidy_check(inkevent) diff --git a/src/iocore/eventsystem/unit_tests/test_Action.cc b/src/iocore/eventsystem/unit_tests/test_Action.cc new file mode 100644 index 00000000000..4201c862992 --- /dev/null +++ b/src/iocore/eventsystem/unit_tests/test_Action.cc @@ -0,0 +1,254 @@ +/** @file + + Catch2 unit tests for the Action boundary contract. + + @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 "inkevent_test_fixtures.h" + +#include + +#include + +using inkevent_test::CountingContinuation; +using inkevent_test::EventProcessorListener; + +CATCH_REGISTER_LISTENER(EventProcessorListener) + +namespace +{ + +class RecordingAction : public Action +{ +public: + using Action::operator=; + + std::atomic derived_cancel_calls{0}; + + void + cancel(Continuation *c = nullptr) override + { + derived_cancel_calls.fetch_add(1, std::memory_order_release); + Action::cancel(c); + } +}; + +} // namespace + +TEST_CASE("A default-constructed Action has a null continuation, a null mutex reference, and a false cancelled flag", + "[inkevent][action]") +{ + Action a; + REQUIRE(a.continuation == nullptr); + REQUIRE(a.mutex.get() == nullptr); + REQUIRE(a.cancelled == false); +} + +TEST_CASE("Action::operator= with a non-null Continuation pins its observable post-conditions", "[inkevent][action]") +{ + Ptr mutex{new_ProxyMutex()}; + CountingContinuation cont{mutex.get()}; + int const before = mutex->refcount(); + Action a; + + Continuation *result = (a = &cont); + + SECTION("the operator returns its argument unchanged so callers can chain assignments") + { + REQUIRE(result == &cont); + } + + SECTION("the Action stores the assigned Continuation pointer and adopts that Continuation's ProxyMutex") + { + REQUIRE(a.continuation == &cont); + REQUIRE(a.mutex.get() == mutex.get()); + } + + SECTION("the Action retains an additional reference to the bound ProxyMutex") + { + REQUIRE(mutex->refcount() == before + 1); + } + + SECTION("the Action's cancelled flag is unchanged by binding a Continuation") + { + REQUIRE(a.cancelled == false); + } +} + +TEST_CASE("Action::operator= with nullptr clears the bound continuation and drops the retained ProxyMutex reference", + "[inkevent][action]") +{ + Ptr mutex{new_ProxyMutex()}; + CountingContinuation cont{mutex.get()}; + Action a; + a = &cont; + int const before_clear = mutex->refcount(); + + a = nullptr; + + REQUIRE(a.continuation == nullptr); + REQUIRE(a.mutex.get() == nullptr); + REQUIRE(mutex->refcount() == before_clear - 1); +} + +TEST_CASE( + "Reassigning Action::operator= to a different Continuation drops the previous ProxyMutex reference and adopts the new one", + "[inkevent][action]") +{ + Ptr mutex1{new_ProxyMutex()}; + Ptr mutex2{new_ProxyMutex()}; + CountingContinuation cont1{mutex1.get()}; + CountingContinuation cont2{mutex2.get()}; + + Action a; + a = &cont1; + int const ref1_one = mutex1->refcount(); + int const ref2_one = mutex2->refcount(); + + a = &cont2; + + REQUIRE(a.continuation == &cont2); + REQUIRE(a.mutex.get() == mutex2.get()); + REQUIRE(mutex1->refcount() == ref1_one - 1); + REQUIRE(mutex2->refcount() == ref2_one + 1); +} + +TEST_CASE( + "Action::operator= with a Continuation that has a null mutex stores the Continuation pointer and leaves Action's mutex null", + "[inkevent][action]") +{ + CountingContinuation cont{nullptr}; + Action a; + + a = &cont; + + REQUIRE(a.continuation == &cont); + REQUIRE(a.mutex.get() == nullptr); +} + +TEST_CASE("Action::cancel flips the cancelled flag to true whether the bound Continuation is passed explicitly or omitted", + "[inkevent][action]") +{ + bool const pass_continuation = GENERATE(true, false); + + Ptr mutex{new_ProxyMutex()}; + CountingContinuation cont{mutex.get()}; + Action a; + a = &cont; + REQUIRE(a.cancelled == false); + + { + SCOPED_MUTEX_LOCK(lock, mutex, this_ethread()); + a.cancel(pass_continuation ? &cont : nullptr); + } + + REQUIRE(a.cancelled == true); +} + +TEST_CASE("Action::cancel dispatches virtually so a derived Action's overridden cancel runs through an Action* base pointer", + "[inkevent][action]") +{ + Ptr mutex{new_ProxyMutex()}; + CountingContinuation cont{mutex.get()}; + RecordingAction derived; + derived = &cont; + Action *base = &derived; + + { + SCOPED_MUTEX_LOCK(lock, mutex, this_ethread()); + base->cancel(&cont); + } + + REQUIRE(derived.derived_cancel_calls.load() == 1); + REQUIRE(derived.cancelled == true); +} + +TEST_CASE("Action::cancel_action sets the cancelled flag without invoking a derived class's overridden cancel", + "[inkevent][action]") +{ + Ptr mutex{new_ProxyMutex()}; + CountingContinuation cont{mutex.get()}; + RecordingAction derived; + derived = &cont; + + { + SCOPED_MUTEX_LOCK(lock, mutex, this_ethread()); + derived.cancel_action(&cont); + } + + REQUIRE(derived.cancelled == true); + REQUIRE(derived.derived_cancel_calls.load() == 0); +} + +TEST_CASE("Destroying an Action that is bound to a Continuation drops its retained ProxyMutex reference", "[inkevent][action]") +{ + Ptr mutex{new_ProxyMutex()}; + CountingContinuation cont{mutex.get()}; + int const before = mutex->refcount(); + + { + Action a; + a = &cont; + REQUIRE(mutex->refcount() == before + 1); + } + + REQUIRE(mutex->refcount() == before); +} + +TEST_CASE("MAKE_ACTION_RESULT encodes a small integer with the low bit set so callers can distinguish sentinels from real Actions", + "[inkevent][action]") +{ + Action *s = MAKE_ACTION_RESULT(7); + REQUIRE((reinterpret_cast(s) & 1u) == 1u); + REQUIRE(reinterpret_cast(s) == ((7u << 1) + 1u)); +} + +TEST_CASE("ACTION_RESULT_DONE and ACTION_IO_ERROR are distinct sentinels with the low bit set", "[inkevent][action]") +{ + REQUIRE((reinterpret_cast(ACTION_RESULT_DONE) & 1u) == 1u); + REQUIRE((reinterpret_cast(ACTION_IO_ERROR) & 1u) == 1u); + REQUIRE(ACTION_RESULT_DONE != ACTION_IO_ERROR); +} + +TEST_CASE("A Continuation scheduled into the future and cancelled before its deadline receives no dispatched event", + "[inkevent][action][multithread]") +{ + Ptr target_mutex{new_ProxyMutex()}; + CountingContinuation target{target_mutex.get()}; + CountingContinuation barrier{new_ProxyMutex()}; + + { + SCOPED_MUTEX_LOCK(lock, target_mutex, this_ethread()); + Action *a = eventProcessor.schedule_in(&target, HRTIME_MSECONDS(20), ET_CALL); + REQUIRE(a != nullptr); + a->cancel(&target); + } + + // Schedule a barrier with a strictly later deadline than target's. The + // EThread cannot dispatch the barrier without first visiting target's slot + // (PriorityEventQueue dequeues by deadline), so awaiting the barrier proves + // the EThread actually ran past target's deadline — without which the + // count() == 0 assertion below would be vacuous on a stalled EThread. + Event *b = eventProcessor.schedule_in(&barrier, HRTIME_MSECONDS(150), ET_CALL); + REQUIRE(b != nullptr); + REQUIRE(barrier.wait_until_at_least(1)); + + REQUIRE(target.count() == 0); +} From 1800702d5522b290cb59e6eb03f5f4c03ba1dc4a Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Thu, 25 Jun 2026 10:34:08 -0500 Subject: [PATCH 2/2] Include atomic header --- src/iocore/eventsystem/unit_tests/test_Action.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iocore/eventsystem/unit_tests/test_Action.cc b/src/iocore/eventsystem/unit_tests/test_Action.cc index 4201c862992..7f8890ad54c 100644 --- a/src/iocore/eventsystem/unit_tests/test_Action.cc +++ b/src/iocore/eventsystem/unit_tests/test_Action.cc @@ -25,7 +25,7 @@ #include -#include +#include using inkevent_test::CountingContinuation; using inkevent_test::EventProcessorListener;