Add unit tests for lock macros

[JosiahWI]

These tests cover the behavior of the event system macros for locking and unlocking ProxyMutex objects.

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds Catch2 unit tests to validate the behavior of Lock.h mutex acquisition/release macros for ProxyMutex in the event system.

Changes:

• Introduces test_Lock.cc covering MUTEX_TRY_LOCK, SCOPED_MUTEX_LOCK, MUTEX_TAKE_LOCK/UNTAKE_LOCK, and weak lock variants (including contention + reentrancy cases).
• Registers the new test target in the eventsystem CMake build when BUILD_TESTING is enabled.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/iocore/eventsystem/unit_tests/test_Lock.cc	Adds Catch2 tests for lock/unlock macros, including scoped/try/weak and contended scenarios.
src/iocore/eventsystem/CMakeLists.txt	Builds and registers the new test_Lock executable under test builds.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:40

• The constructor takes Ptr<ProxyMutex> &target but does not mutate it. Prefer Ptr<ProxyMutex> const& (or pass by value and std::move into target_mutex) to better communicate intent and avoid requiring an lvalue at call sites.

  HoldOnEThread(ProxyMutex *self_mutex, Ptr<ProxyMutex> &target) : Continuation(self_mutex), target_mutex(target)
  {
    SET_HANDLER(&HoldOnEThread::on_event);
  }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:61

• HoldOnEThread::on_event ignores the return value of release.wait_until_set(). If that wait times out, the handler will still proceed, unlock the mutex, and set done, which can make the contention assertions flaky (e.g., MUTEX_TRY_LOCK might succeed because the holder timed out and released early). Treat a timeout as a test failure by not setting done when the release signal was never observed (and consider using a longer timeout than the default).

    {
      SCOPED_MUTEX_LOCK(guard, target_mutex, this_ethread());
      held.set();
      release.wait_until_set();
    }
    done.set();
    return 0;

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/iocore/eventsystem/unit_tests/test_Lock.cc:58

• Using a fatal Catch2 assertion (REQUIRE) inside the scheduled continuation can throw and skip the subsequent done.set(), which then forces the main thread to wait for a timeout before failing (and can leave the event thread in a bad state). Prefer a non-fatal assertion here so done is always signaled.

      REQUIRE(release.wait_until_set());

src/iocore/eventsystem/unit_tests/test_Lock.cc:44

• The comment above the destructor is misleading: release is signaled by the test thread (the one driving the assertions), not by the scheduled event thread. Clarifying this makes it easier to understand what failure mode this destructor is guarding against.

  // In case of an exception in a thread that would have set release, we set
  // it here in order to unfreeze any threads that may be waiting on done.
  ~HoldOnEThread() { release.set(); }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/iocore/eventsystem/unit_tests/test_Lock.cc:92

• If any of the fatal REQUIRE(...) checks before holder.release.set() fail (e.g. held.wait_until_set() timing out), stack unwinding will destroy holder while the eventProcessor may still dispatch the scheduled event, risking a use-after-free / crash in the test executable. Add a small RAII cleanup object after scheduling to always signal release and wait for done during unwind, so the scheduled Continuation can’t outlive its storage.

  REQUIRE(eventProcessor.schedule_imm(&holder, ET_CALL) != nullptr);
  REQUIRE(holder.held.wait_until_set());

  EThread *t = this_ethread();
  MUTEX_TRY_LOCK(guard, contended, t);

src/iocore/eventsystem/unit_tests/test_Lock.cc:267

• Same lifetime hazard as above: if a fatal assertion throws before reaching the explicit holder.release.set() / holder.done.wait_until_set(), the scheduled event can still run against a destroyed stack holder. Add a local RAII cleanup object right after scheduling to guarantee release + done synchronization during stack unwinding.

  REQUIRE(eventProcessor.schedule_imm(&holder, ET_CALL) != nullptr);
  REQUIRE(holder.held.wait_until_set());

  holder.release.set();
  REQUIRE(holder.done.wait_until_set());

src/iocore/eventsystem/unit_tests/test_Lock.cc:325

• Same issue here: the continuation is stack-allocated, but the scheduled event may still execute if a REQUIRE(...) throws before cleanup runs, which can lead to a use-after-free in the test process. Add an RAII cleanup guard after scheduling so release is signaled and done is awaited even during stack unwinding.

  REQUIRE(eventProcessor.schedule_imm(&holder, ET_CALL) != nullptr);
  REQUIRE(holder.held.wait_until_set());

  EThread *t = this_ethread();
  WEAK_MUTEX_TRY_LOCK(guard, contended, t);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:256

• This re-entrancy test takes the mutex twice, but the two MUTEX_UNTAKE_LOCK calls are not exception-safe: any failing REQUIRE between the takes and untakes will skip cleanup during stack unwinding, potentially destroying a still-locked ink_mutex (ProxyMutex::free() calls ink_mutex_destroy). Use a small local RAII helper that tracks the take count and guarantees all outstanding untakes happen in its destructor.

  MUTEX_TAKE_LOCK(m, t);
  MUTEX_TAKE_LOCK(m, t);

  REQUIRE(m->nthread_holding == 2);
  REQUIRE(m->thread_holding == t);

  MUTEX_UNTAKE_LOCK(m, t);
  MUTEX_UNTAKE_LOCK(m, t);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:49

• HoldOnEThread is stack-allocated but is scheduled onto the event thread. If holder.held.wait_until_set() (or any other REQUIRE before holder.done.wait_until_set()) times out / fails, stack unwinding will destroy holder while its Event may still be queued. The destructor only waits up to DEFAULT_TIMEOUT (5s) and does not cancel the scheduled event, so a delayed dispatch can become a use-after-free when the event thread later calls back into the destroyed continuation.

  ~HoldOnEThread()
  {
    release.set();
    done.wait_until_set();
  }

[masaori335]

[approve ci]

[masaori335]

This looks flaky. If I run this test repeatedly like below, it fails. Is it expected?

ctest --test-dir build-asf-master-review-0-debug -R '^test_Lock$' --repeat until-fail:100 --output-on-failure

[JosiahWI]

@masaori335 I have cleaned up the synchronization. I ran your command and it passed. I've also run the test through ASan and TSan. Curiously, TSan detects a data race on ProxyMutex::thread_holding in debug builds (not in release builds). Sure enough, accesses to ProxyMutex::thread_holding in the locking code are unsynchronized.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:99

• callback_action is dereferenced without a null check. If eventProcessor.schedule_imm(...) can ever return nullptr (e.g., scheduling failure during test initialization/teardown), this will crash. Consider asserting non-null immediately after scheduling (or guarding here) so test failures are reported cleanly rather than via a null dereference.

  bool
  is_expecting_callback()
  {
    return !this->held.is_set() && !this->callback_action->cancelled;
  }

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

include/iocore/eventsystem/Action.h:256

• The PR description/title focus on adding unit tests for lock macros, but this change also introduces a public sentinel macro (ACTION_IO_ERROR) and extensive Action API documentation updates. If the macro is intentionally being (re)exported, it should be called out in the PR description (and ideally justified, since it can affect downstream compilation/behavior); otherwise, consider splitting Action.h changes into a separate PR to keep scope aligned.

#define ACTION_RESULT_DONE MAKE_ACTION_RESULT(1)

include/iocore/eventsystem/Action.h:269

• The PR description/title focus on adding unit tests for lock macros, but this change also introduces a public sentinel macro (ACTION_IO_ERROR) and extensive Action API documentation updates. If the macro is intentionally being (re)exported, it should be called out in the PR description (and ideally justified, since it can affect downstream compilation/behavior); otherwise, consider splitting Action.h changes into a separate PR to keep scope aligned.

#define ACTION_IO_ERROR MAKE_ACTION_RESULT(2)

include/iocore/eventsystem/Action.h:301

• _x is not parenthesized inside the macro. If callers pass an expression with lower-precedence operators (e.g. _x expands to a & 1), the shift can bind unexpectedly (a & (1 << 1)), producing the wrong sentinel value. Wrap _x in parentheses in the shift expression to preserve intended semantics (and keep the rest of the expression fully parenthesized).

#define MAKE_ACTION_RESULT(_x) (Action *)(((uintptr_t)((_x << 1) + 1)))

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

src/iocore/eventsystem/unit_tests/test_Lock.cc:68

• The contended-lock tests are potentially flaky because HoldOnEThread::on_event() may stop holding target_mutex if release.wait_until_set() times out, which would make MUTEX_TRY_LOCK sometimes succeed unexpectedly. To keep these multithread tests deterministic, ensure the holder callback blocks until explicitly released (i.e., avoid a timeout-based wait in the holder), or treat any wait timeout as a hard failure that prevents the test from proceeding.

    // The callback can finish without setting done due to wait timeouts. We
    // return false in that case.
    return this->done.wait_until_set();

src/iocore/eventsystem/unit_tests/test_Lock.cc:137

• The contended-lock tests are potentially flaky because HoldOnEThread::on_event() may stop holding target_mutex if release.wait_until_set() times out, which would make MUTEX_TRY_LOCK sometimes succeed unexpectedly. To keep these multithread tests deterministic, ensure the holder callback blocks until explicitly released (i.e., avoid a timeout-based wait in the holder), or treat any wait timeout as a hard failure that prevents the test from proceeding.

TEST_CASE("MUTEX_TRY_LOCK against a contended ProxyMutex constructs a guard whose is_locked() reports the failed acquisition",
          "[inkevent][lock][multithread]")
{
  Ptr<ProxyMutex> contended{new_ProxyMutex()};
  Ptr<ProxyMutex> cont_self{new_ProxyMutex()};
  HoldOnEThread   holder{cont_self.get(), contended};

  REQUIRE(holder.wait_for_callback_start());

  EThread *t = this_ethread();
  MUTEX_TRY_LOCK(guard, contended, t);

  REQUIRE_FALSE(guard.is_locked());
}

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:107

• Encoding state in the low bit of a pointer is brittle and non-obvious in a unit test helper (it relies on alignment/representation details and makes the test harder to maintain). Suggestion (moderate): replace this with an explicit API/state check (e.g., an Action method/flag that indicates completion/cancellation) or wrap the “encoded action” concept behind a named helper function with a comment referencing the underlying convention.

    if (reinterpret_cast<std::uintptr_t>(this->callback_action) & 1) {
      return false;
    }

[JosiahWI]

The parent selection regression test failed on Debian.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/iocore/eventsystem/unit_tests/test_Lock.cc:111

• The pointer-bit check (uintptr_t(callback_action) & 1) is a brittle dependency on an internal/implicit representation (tagged pointers or sentinel values). For test synchronization, it would be more robust to avoid inspecting pointer bits and instead track state explicitly (e.g., null out callback_action when it becomes invalid, or add/drive a dedicated atomic flag that represents 'callback scheduled/started/cancelled'). This reduces the chance of false behavior on different platforms/ABIs and makes the intent clearer.

  is_expecting_callback()
  {
    if (reinterpret_cast<std::uintptr_t>(this->callback_action) & 1) {
      return false;
    }

    ink_assert(this->mutex->thread_holding == this_ethread());
    return !this->held.is_set() && !this->callback_action->cancelled;
  }