From 2ff4d388327cc1c548eca0802e220a233edd134c Mon Sep 17 00:00:00 2001 From: DMG Date: Fri, 5 Apr 2024 12:33:15 -0700 Subject: [PATCH 1/3] Added two related fixes to "Entry* pop()" in mailbox.h (pop-from-queue method). - Address case where a push happens in another thread between the Queue::pop() and the event_fd read. - Address case where two event_fd writes in other thread arrive right at the moment when the event_fd read is happening. These fixes address a network connection hang revealed by the test multiple_client_with_requests_to_multithreaded_server when running run_http_server_test over and over again. --- include/pistache/mailbox.h | 39 +++++++++++++++++++++++++++++++++++++- version.txt | 2 +- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/include/pistache/mailbox.h b/include/pistache/mailbox.h index 26b0d84d1..e149ba6f9 100644 --- a/include/pistache/mailbox.h +++ b/include/pistache/mailbox.h @@ -311,10 +311,47 @@ namespace Pistache if (isBound()) { - uint64_t val; + uint64_t val = 0; for (;;) { ssize_t bytes = read(event_fd, &val, sizeof val); + + // Note: Without the read-success check below, there can be + // an issue that shows up in the test + // multiple_client_with_requests_to_multithreaded_server + // when calling run_http_server_test over and over again. + // + // Without the check, in a typical success case the read in + // this function would be called twice. First time, read + // reads val. Second time, read fails with errno = EAGAIN / + // EWOULDBLOCK, causing us to break out of the loop and + // return from our "pop" function here. + // + // However, in the problem case, very occasionally two + // pushes - and hence two writes to event_fd - occur just + // ahead of the read, and both writes succeed by the time + // we are calling read the second time in the for(;;) + // loop. This causes the _second_ read to succeeed, which + // clears the eventfd readiness caused by the write, even + // though we have yet to pop that push off the queue. Hence + // the values that were pushed might never be processed off + // of this queue. So, we should break out of the loop when + // the read succeeds. + if (bytes == (sizeof val)) + { // success + + if (!ret) + { + // Have another try at pop, in case there was no + // "pop" to do at the top of this function, but + // then a push happened after the first pop attempt + // but before the read above + ret = Queue::pop(); + } + + break; + } + if (bytes == -1) { if (errno == EAGAIN || errno == EWOULDBLOCK) diff --git a/version.txt b/version.txt index 507fe4874..8dbc7056d 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.2.7.20240223 +0.2.7.20240405 From 1257d8fa4f15a635ca5f21e0a2a7184e8278730c Mon Sep 17 00:00:00 2001 From: DMG Date: Fri, 5 Apr 2024 13:00:09 -0700 Subject: [PATCH 2/3] Added test many_client_with_requests_to_multithreaded_server in the hope of triggering the "Entry pop" bug more frequently. Not sure the error condition does actually trigger more frequently, but seems a worthwhile test to include either way. --- tests/http_server_test.cc | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/http_server_test.cc b/tests/http_server_test.cc index 94b20585c..317d1dffe 100644 --- a/tests/http_server_test.cc +++ b/tests/http_server_test.cc @@ -345,6 +345,42 @@ TEST(http_server_test, multiple_client_with_requests_to_multithreaded_server) ASSERT_EQ(res2, SECOND_CLIENT_REQUEST_SIZE); } +TEST(http_server_test, many_client_with_requests_to_multithreaded_server) +{ + const Pistache::Address address("localhost", Pistache::Port(0)); + + Http::Endpoint server(address); + auto flags = Tcp::Options::ReuseAddr; + auto server_opts = Http::Endpoint::options().flags(flags).threads(6); + server.init(server_opts); + LOGGER("test", "Trying to run server..."); + server.setHandler(Http::make_handler()); + ASSERT_NO_THROW(server.serveThreaded()); + + const std::string server_address = "localhost:" + server.getPort().toString(); + LOGGER("test", "Server address: " << server_address); + + const int NO_TIMEOUT = 0; + const int SECONDS_TIMOUT = 10; + const int FIRST_CLIENT_REQUEST_SIZE = 128; + std::future result1(std::async(clientLogicFunc, + FIRST_CLIENT_REQUEST_SIZE, server_address, + NO_TIMEOUT, SECONDS_TIMOUT)); + const int SECOND_CLIENT_REQUEST_SIZE = 192; + std::future result2( + std::async(clientLogicFunc, SECOND_CLIENT_REQUEST_SIZE, server_address, + NO_TIMEOUT, SECONDS_TIMOUT)); + + int res1 = result1.get(); + int res2 = result2.get(); + + server.shutdown(); + + ASSERT_EQ(res1, FIRST_CLIENT_REQUEST_SIZE); + ASSERT_EQ(res2, SECOND_CLIENT_REQUEST_SIZE); +} + + TEST(http_server_test, multiple_client_with_different_requests_to_multithreaded_server) { From c862b58be23f618152a010c1f91225e2afa2e9e5 Mon Sep 17 00:00:00 2001 From: DMG Date: Fri, 5 Apr 2024 13:03:27 -0700 Subject: [PATCH 3/3] Conformed code formatting --- tests/http_server_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/http_server_test.cc b/tests/http_server_test.cc index 317d1dffe..0032bd36c 100644 --- a/tests/http_server_test.cc +++ b/tests/http_server_test.cc @@ -361,7 +361,7 @@ TEST(http_server_test, many_client_with_requests_to_multithreaded_server) LOGGER("test", "Server address: " << server_address); const int NO_TIMEOUT = 0; - const int SECONDS_TIMOUT = 10; + const int SECONDS_TIMOUT = 10; const int FIRST_CLIENT_REQUEST_SIZE = 128; std::future result1(std::async(clientLogicFunc, FIRST_CLIENT_REQUEST_SIZE, server_address, @@ -380,7 +380,6 @@ TEST(http_server_test, many_client_with_requests_to_multithreaded_server) ASSERT_EQ(res2, SECOND_CLIENT_REQUEST_SIZE); } - TEST(http_server_test, multiple_client_with_different_requests_to_multithreaded_server) { @@ -929,7 +928,8 @@ struct ContentEncodingHandler : public Http::Handler writer.setCompression(encoding); // Set compression level... - switch(encoding) { + switch (encoding) + { #ifdef PISTACHE_USE_CONTENT_ENCODING_BROTLI // Set maximum compression if using Brotli