Skip to content

Commit

Permalink
Fix promise sync in bool_waiter; rename to callback_waiter
Browse files Browse the repository at this point in the history 
We were setting the promise in bool_waiter *before* we call the wrapped
lambda, but that introduces a race condition because we have test code
that waits on the promise and then checks a value that it expects to
have been set inside the lambda.

This reverses it so that the promise is set *after* the lambda is
called.

While here, I also refactored this to a `promise<void>` since we are
never actually meaningfully using the `bool` value (which either returns
true, or hangs), and then since it isn't a "bool" waiter anymore,
renamed to callback_waiter.

Also documented its usage.
  • Loading branch information
@jagerman
jagerman committed Sep 21, 2023
1 parent ab17d20 commit 415e306
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 36 deletions.
13 changes: 5 additions & 8 deletions tests/001-handshake.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ namespace oxen::quic::test

SECTION("Endpoint::connect() - IPv6 Addressing")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto server_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};
auto server_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand All @@ -135,15 +135,14 @@ namespace oxen::quic::test
REQUIRE_NOTHROW(client_endpoint->connect(client_remote, client_tls));

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.get());
REQUIRE(server_established.get());
REQUIRE(server_established.wait_ready());
};
};

TEST_CASE("001 - Handshaking: Execution", "[001][handshake][tls][execute]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto server_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};
auto server_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand All @@ -163,7 +162,5 @@ namespace oxen::quic::test

REQUIRE(client_established.wait_ready());
REQUIRE(server_established.wait_ready());
REQUIRE(client_established.get());
REQUIRE(server_established.get());
};
} // namespace oxen::quic::test
6 changes: 3 additions & 3 deletions tests/004-streams.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace oxen::quic::test

TEST_CASE("004 - Multiple pending streams: max stream count", "[004][streams][pending][config]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down Expand Up @@ -72,7 +72,7 @@ namespace oxen::quic::test

TEST_CASE("004 - Multiple pending streams: different remote settings", "[004][streams][pending][config]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};
auto msg = "hello from the other siiiii-iiiiide"_bsv;
Expand Down Expand Up @@ -127,7 +127,7 @@ namespace oxen::quic::test

TEST_CASE("004 - Multiple pending streams: Execution", "[004][streams][pending][execute]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};
auto msg = "hello from the other siiiii-iiiiide"_bsv;
Expand Down
20 changes: 10 additions & 10 deletions tests/007-datagrams.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ namespace oxen::quic::test

TEST_CASE("007 - Datagram support: Query param info from datagram-disabled endpoint", "[007][datagrams][types]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand All @@ -91,7 +91,7 @@ namespace oxen::quic::test

TEST_CASE("007 - Datagram support: Query param info from default datagram-enabled endpoint", "[007][datagrams][types]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down Expand Up @@ -122,7 +122,7 @@ namespace oxen::quic::test

TEST_CASE("007 - Datagram support: Query params from split-datagram enabled endpoint", "[007][datagrams][types]")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand All @@ -141,7 +141,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, split_dgram, client_established);
auto conn_interface = client_endpoint->connect(client_remote, client_tls);

REQUIRE(client_established.get());
REQUIRE(client_established.wait_ready());
REQUIRE(conn_interface->datagrams_enabled());
REQUIRE(conn_interface->packet_splitting_enabled());

Expand All @@ -153,7 +153,7 @@ namespace oxen::quic::test
{
SECTION("Simple datagram transmission")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};
auto msg = "hello from the other siiiii-iiiiide"_bsv;
Expand Down Expand Up @@ -203,7 +203,7 @@ namespace oxen::quic::test
{
SECTION("Simple datagram transmission")
{
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down Expand Up @@ -270,7 +270,7 @@ namespace oxen::quic::test
SECTION("Simple oversized datagram transmission - Clear first row")
{
log::trace(log_cat, "Beginning the unit test from hell");
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down Expand Up @@ -355,7 +355,7 @@ namespace oxen::quic::test
SECTION("Simple datagram transmission - mixed sizes")
{
log::trace(log_cat, "Beginning the unit test from hell");
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down Expand Up @@ -443,7 +443,7 @@ namespace oxen::quic::test
SECTION("Simple datagram transmission - induced loss")
{
log::trace(log_cat, "Beginning the unit test from hell");
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down Expand Up @@ -539,7 +539,7 @@ namespace oxen::quic::test
SECTION("Simple datagram transmission - flip flop ordering")
{
log::trace(log_cat, "Beginning the unit test from hell");
auto client_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};

Network test_net{};

Expand Down
12 changes: 6 additions & 6 deletions tests/009-close.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ namespace oxen::quic::test
{
uint64_t client_error{0};
uint64_t server_error{0};
auto client_established = bool_waiter{[](connection_interface&) {}};
auto server_established = bool_waiter{[](connection_interface&) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};
auto server_established = callback_waiter{[](connection_interface&) {}};
// this needs to be destroyed *after* Network, as it may be called during ~Network
auto client_closed = bool_waiter{[&client_error](connection_interface&, uint64_t ec) { client_error = ec; }};
auto server_closed = bool_waiter{[&server_error](connection_interface&, uint64_t ec) { server_error = ec; }};
auto client_closed = callback_waiter{[&client_error](connection_interface&, uint64_t ec) { client_error = ec; }};
auto server_closed = callback_waiter{[&server_error](connection_interface&, uint64_t ec) { server_error = ec; }};

Network test_net{};

Expand Down Expand Up @@ -45,8 +45,8 @@ namespace oxen::quic::test

REQUIRE(server_closed.wait_ready());
REQUIRE(client_closed.wait_ready());
REQUIRE(client_error == error_code);
REQUIRE(server_error == error_code);
CHECK(client_error == error_code);
CHECK(server_error == error_code);
};
};
} // namespace oxen::quic::test
50 changes: 41 additions & 9 deletions tests/utils.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,37 +119,69 @@ namespace oxen::quic
template <typename Class, typename Ret, typename... Args>
struct functional_helper<Ret (Class::*)(Args...) const>
{
using return_type = Ret;
static constexpr bool is_void = std::is_void_v<Ret>;
using type = std::function<Ret(Args...)>;
};

template <typename T>
using functional_helper_t = typename functional_helper<T>::type;

struct set_on_exit
{
std::promise<void>& p;
explicit set_on_exit(std::promise<void>& p) : p{p} {}
~set_on_exit() { p.set_value(); }
};

/// Test suite helper that takes a callable lambda at construction and then man-in-the-middles
/// an intermediate std::function matching the lambda that calls the inner lambda but also sets
/// a promise just after calling the inner lambda.
///
/// The main purpose is to synchronize an asynchronous interface with a promise/future to
/// simplify test code which is full of "wait for this thing to be called" checks, without
/// needing any sort of sleep & poll (and reducing the direct usage of promise/futures in the
/// test suite).
///
/// Usage example:
///
/// int foo = 0;
/// callback_waiter waiter{[&foo](int a, int b) { foo = a + b; }};
/// invoke_something(waiter);
///
/// where `invoke_something` takes a `std::function<int(Foo&, int)>`. The test code would then
/// go on to synchronize with:
///
/// REQUIRE(waiter.wait_ready()); // will fail if the lambda doesn't get called with ~1s
///
/// and then can go on to check side effects of the lambda, e.g.:
///
/// CHECK(foo == 42);
///
/// Care must be taken to ensure the lambda is only called once. The lambda may throw, but the
/// throw propagates to the caller of the lambda, *not* the inner promise.
template <typename T>
struct bool_waiter
struct callback_waiter
{
using Func_t = functional_helper_t<T>;

Func_t func;
std::promise<bool> p;
std::future<bool> f{p.get_future()};
std::promise<void> p;
std::future<void> f{p.get_future()};

explicit bool_waiter(T f) : func{std::move(f)} {}
explicit callback_waiter(T f) : func{std::move(f)} {}

bool wait_ready(std::chrono::milliseconds timeout = 1s) { return f.wait_for(timeout) == std::future_status::ready; }

bool is_ready() { return f.wait_for(0s) == std::future_status::ready; }

bool get() { return f.get(); }
bool is_ready() { return wait_ready(0s); }

// Deliberate implicit conversion to the std::function<...>
operator Func_t()
{
return [this](auto&&... args) {
p.set_value(true);
set_on_exit prom_setter{p};
return func(std::forward<decltype(args)>(args)...);
};
return func;
}
};

Expand Down

0 comments on commit 415e306

Please sign in to comment.