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, which should solve the intermittent failure of the 009 test
caused by exactly this race condition.

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.

`get` doesn't seem useful (it basically is a return-or-hang), so I removed it.

`wait_ready()` felt too long, so I shortened it to `wait()`.

Also adds documentation of the usage of `bool_waiter`.
  • Loading branch information
@jagerman
jagerman committed Sep 22, 2023
1 parent 52c4b2e commit fe1a5f9
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 71 deletions.
19 changes: 8 additions & 11 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 @@ -134,16 +134,15 @@ 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(client_established.wait());
REQUIRE(server_established.wait());
};
};

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 @@ -161,9 +160,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto conn_interface = client_endpoint->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(server_established.wait_ready());
REQUIRE(client_established.get());
REQUIRE(server_established.get());
REQUIRE(client_established.wait());
REQUIRE(server_established.wait());
};
} // namespace oxen::quic::test
20 changes: 10 additions & 10 deletions tests/002-send-receive.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ namespace oxen::quic::test

SECTION("Client sends a command")
{
auto server_bp_cb = bool_waiter{[&](message msg) {
auto server_bp_cb = callback_waiter{[&](message msg) {
if (msg)
log::info(log_cat, "Server bparser received: {}", msg.view());
}};
Expand All @@ -184,20 +184,20 @@ namespace oxen::quic::test

client_bp->command("test_endpoint"s, "test_request_body"s);

REQUIRE(server_bp_cb.get());
REQUIRE(server_bp_cb.wait());
}

SECTION("Client sends a request, server sends a response")
{
auto server_bp_cb = bool_waiter{[&](message msg) {
auto server_bp_cb = callback_waiter{[&](message msg) {
if (msg)
{
log::info(log_cat, "Server bparser received: {}", msg.view());
msg.respond(msg.rid(), "test_response"s);
}
}};

auto client_bp_cb = bool_waiter{[&](message msg) {
auto client_bp_cb = callback_waiter{[&](message msg) {
if (msg)
{
log::info(log_cat, "Client bparser received: {}", msg.view());
Expand Down Expand Up @@ -225,21 +225,21 @@ namespace oxen::quic::test

client_bp->request("test_endpoint"s, "test_request_body"s);

REQUIRE(server_bp_cb.get());
REQUIRE(client_bp_cb.get());
REQUIRE(server_bp_cb.wait());
REQUIRE(client_bp_cb.wait());
}

SECTION("Client (alternate construction) sends a request, server sends a response")
{
auto server_bp_cb = bool_waiter{[&](message msg) {
auto server_bp_cb = callback_waiter{[&](message msg) {
if (msg)
{
log::info(log_cat, "Server bparser received: {}", msg.view());
msg.respond(msg.rid(), "test_response"s);
}
}};

auto client_bp_cb = bool_waiter{[&](message msg) {
auto client_bp_cb = callback_waiter{[&](message msg) {
if (msg)
{
log::info(log_cat, "Client bparser received: {}", msg.view());
Expand All @@ -263,8 +263,8 @@ namespace oxen::quic::test

client_bp->request("test_endpoint"s, "test_request_body"s);

REQUIRE(server_bp_cb.get());
REQUIRE(client_bp_cb.get());
REQUIRE(server_bp_cb.wait());
REQUIRE(client_bp_cb.wait());
}
};

Expand Down
24 changes: 12 additions & 12 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 All @@ -31,7 +31,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto conn_interface = client_endpoint->connect(client_remote, client_tls, max_streams);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());
REQUIRE(conn_interface->get_max_streams() == max_streams.stream_count);
};

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 @@ -102,7 +102,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto client_ci = client_endpoint->connect(client_remote, client_tls, client_config);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

server_ci = server_endpoint->get_all_conns(Direction::INBOUND).front();
// some transport parameters are set after handshake is completed; querying the client connection too
Expand All @@ -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 Expand Up @@ -179,7 +179,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto conn_interface = client_endpoint->connect(client_remote, client_tls, max_streams);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

for (int i = 0; i < n_streams; ++i)
{
Expand Down Expand Up @@ -418,8 +418,8 @@ namespace oxen::quic::test
std::shared_ptr<CustomStreamB> server_b, client_b;
std::shared_ptr<CustomStreamC> server_c, client_c;

auto client_established = bool_waiter{[](connection_interface&) {}};
auto server_closed = bool_waiter{[](connection_interface&, uint64_t) {}};
auto client_established = callback_waiter{[](connection_interface&) {}};
auto server_closed = callback_waiter{[](connection_interface&, uint64_t) {}};

SECTION("Stream logic using connection open callback")
{
Expand All @@ -446,7 +446,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto client_ci = client_endpoint->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

log::info(bp_cat, "Client opening Custom Stream A!");
client_a = client_ci->get_new_stream<CustomStreamA>(std::move(cp1));
Expand All @@ -464,7 +464,7 @@ namespace oxen::quic::test
REQUIRE(sf3.get());

client_ci->close_connection();
REQUIRE(server_closed.get());
REQUIRE(server_closed.wait());
}

SECTION("Stream logic using stream constructor callback")
Expand Down Expand Up @@ -505,7 +505,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto client_ci = client_endpoint->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

log::info(bp_cat, "Client opening Custom Stream A!");
client_a = client_ci->get_new_stream<CustomStreamA>(std::move(cp1));
Expand All @@ -523,7 +523,7 @@ namespace oxen::quic::test
REQUIRE(sf3.get());

client_ci->close_connection();
REQUIRE(server_closed.get());
REQUIRE(server_closed.wait());
}
};

Expand Down
36 changes: 18 additions & 18 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 @@ -82,7 +82,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, client_established);
auto conn_interface = client_endpoint->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());
REQUIRE_FALSE(conn_interface->datagrams_enabled());
REQUIRE_FALSE(conn_interface->packet_splitting_enabled());
REQUIRE_FALSE(conn_interface->packet_splitting_enabled());
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 All @@ -111,7 +111,7 @@ namespace oxen::quic::test
auto client_endpoint = test_net.endpoint(client_local, default_gram, client_established);
auto conn_interface = client_endpoint->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());
REQUIRE(conn_interface->datagrams_enabled());
REQUIRE_FALSE(conn_interface->packet_splitting_enabled());
REQUIRE_FALSE(conn_interface->packet_splitting_enabled());
Expand All @@ -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());
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 @@ -183,7 +183,7 @@ namespace oxen::quic::test
auto client = test_net.endpoint(client_local, default_gram, client_established);
auto conn_interface = client->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());
REQUIRE(server_endpoint->datagrams_enabled());
REQUIRE(client->datagrams_enabled());

Expand All @@ -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 @@ -234,7 +234,7 @@ namespace oxen::quic::test
auto client = test_net.endpoint(client_local, split_dgram, client_established);
auto conn_interface = client->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());
REQUIRE(server_endpoint->datagrams_enabled());
REQUIRE(client->datagrams_enabled());

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 @@ -315,7 +315,7 @@ namespace oxen::quic::test
auto client = test_net.endpoint(client_local, split_dgram, client_established);
auto conn_interface = client->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

REQUIRE(server_endpoint->datagrams_enabled());
REQUIRE(client->datagrams_enabled());
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 @@ -400,7 +400,7 @@ namespace oxen::quic::test
auto client = test_net.endpoint(client_local, split_dgram, client_established);
auto conn_interface = client->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

REQUIRE(server_endpoint->datagrams_enabled());
REQUIRE(client->datagrams_enabled());
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 @@ -492,7 +492,7 @@ namespace oxen::quic::test
auto client = test_net.endpoint(client_local, split_dgram, client_established);
auto conn_interface = client->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

auto server_ci = server_endpoint->get_all_conns(Direction::INBOUND).front();

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 Expand Up @@ -585,7 +585,7 @@ namespace oxen::quic::test
auto client = test_net.endpoint(client_local, split_dgram, client_established);
auto conn_interface = client->connect(client_remote, client_tls);

REQUIRE(client_established.wait_ready());
REQUIRE(client_established.wait());

std::this_thread::sleep_for(5ms);
auto max_size = conn_interface->get_max_datagram_size();
Expand Down

0 comments on commit fe1a5f9

Please sign in to comment.