base tests on raft_fixture

[bashtanov]

Improve raft_fixture and stm_raft_fixture, base mux_state_machine_test and tm_stm_tests on them.

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v24.1.x
• v23.3.x
• v23.2.x

Release Notes

• none

[bashtanov]

vbuild/debug/clang/bin/gtest_raft_rpunit -- --gtest_filter=kv1_stm_fixture.test_mux_state_machine_simple_scenarios to reproduce the problem

[bashtanov]

/dt

[bashtanov]

/dt

[bharathv]

:D easy on the eyes.

[bashtanov]

Really waiting for C++23 to express compile-time inheritance as easily as runtime

[bharathv]

nit: I wonder if this should be a forward or a move

[bashtanov]

I think it should be a forward. The lazy monkey argument would be that all subsequent calls forward it, so why should we break this chain? Really, they seem to do it for a reason, the reason being do_with actually working "well" with lvalue references. It accepts an lvalue reference and never copies or moves the object. In this example the object is not copiable, and after calling with an lvalue reference we can check it has not been moved-from either:

struct kv1_stm_fixture : kv_stm_fixture<kv1_stm_t> {
    ...
    ss::future<int> foo_rvref() {
        std::unique_ptr<int> uptr = std::make_unique<int>(5);
        return ss::do_with(std::move(uptr), [](const std::unique_ptr<int>& p) {
            return foo_impl(p);
        });
    }

     // the test fails with `AddressSanitizer: stack-use-after-return` if we declare it in the function body
    std::unique_ptr<int> member_uptr = std::make_unique<int>(5);
    ss::future<int> foo_lvref() {
        return ss::do_with(
          std::ref(member_uptr),
          [](const std::unique_ptr<int>& p) { return foo_impl(p); });
    }

    static ss::future<int>
    foo_impl(const std::unique_ptr<int>& non_copiable_parameter) {
        co_await ss::sleep(1s);
        vlog(kvlog.info, "lv");
        co_return *non_copiable_parameter.get();
    }

    static ss::future<int>
    foo_impl(std::unique_ptr<int>&& non_copiable_parameter) {
        co_await ss::sleep(1s);
        vlog(kvlog.info, "rv");
        co_return *std::move(non_copiable_parameter).release();
    }
};

TEST_F_CORO(kv1_stm_fixture, test_mux_state_machine_simple_scenarios) {
    ASSERT_EQ_CORO(co_await foo_lvref(), 5);
    ASSERT_TRUE_CORO(member_uptr);
    ASSERT_EQ_CORO(co_await foo_rvref(), 5);
...
}

I put "well" in quotes because, when called with an lvalue reference, do_with does not attempt to extend the object lifetime (and it's not that it has a chance to). So, despite do_with it remains the caller's responsibility to ensure the object is alive until the future completion.

I find it handy, as it allows us code universal functions like retry_with_leader to accept lvalue references to long-life objects as well as rvalue references to temporaries. We don't have to code separate versions of such functions, one with do_with and another one without, we just need to forward in there.

The only problem is that this behaviour of do_with is undocumented.

[bharathv]

I put "well" in quotes because, when called with an lvalue reference, do_with does not attempt to extend the object lifetime (and it's not that it has a chance to). So, despite do_with it remains the caller's responsibility to ensure the object is alive until the future completion.

Right.. I was wondering whether we should disallow lvalue refs here to avoid this footgun. To your point that it is handy to just forward and let the callers handle the lifetime seems like a fair argument, especially in this test code.

[bashtanov]

TBH I only checked forwarding/moving of arguments, not the function itself. But I think there's no harm in making them behave consistently as, eventually they anyway form a closure together.