From 721fc7126641a80e3c67de5158b6bc10bd0a1b70 Mon Sep 17 00:00:00 2001 From: DMG Date: Fri, 30 Aug 2024 10:22:44 -0700 Subject: [PATCH 1/3] Fix: In Test Code, Save Errno Before Calling strerror Issue: In the macOS github workflow, there was an intermittent failure for the test http_server_test / client_request_timeout_on_delay_in_request_line_send_raises_http_408. The test does a repeated ::send until a failure, expecting the error code (errno saved to lastErrno_) to be EPIPE; in the failing case, lastErrno_ is not EPIPE but zero. Examining the code, the macro CLIENT_TRY was calling strerror to get an error string from errno BEFORE doing lastErrno_ = errno; however this is technically incorrect. strerror is actually allowed to modify errno - which could result in lastErrno_ being incorrect (lastErrno_ would hold the errno set by strerror, not the value of errno set by the prior failing operation). Change: in CLIENT_TRY we now do "lastErrno_ = errno" before calling strerror. We also check for errno being zero even before strerror is called (because the prior operation never set errno in the first place), and, for information only, we check to see if strerror does in practice modify errno and send a note to cout if so (if strerror does modify errno it won't affect the subsequent code path given the change described, but it may be interesting to know nonetheless). --- .github/workflows/macos.yaml | 2 ++ tests/tcp_client.h | 46 ++++++++++++++++++++++++++++-------- version.txt | 2 +- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/.github/workflows/macos.yaml b/.github/workflows/macos.yaml index 4f3deb79f..dbda3602c 100644 --- a/.github/workflows/macos.yaml +++ b/.github/workflows/macos.yaml @@ -13,10 +13,12 @@ on: branches: - master - macOSRunner + - ErrnoNotEPIPEServerTest pull_request: branches: - master - macOSRunner + - ErrnoNotEPIPEServerTest defaults: run: diff --git a/tests/tcp_client.h b/tests/tcp_client.h index e2a51ffc3..b49ddeff0 100644 --- a/tests/tcp_client.h +++ b/tests/tcp_client.h @@ -14,19 +14,45 @@ #include #include +// In CLIENT_TRY, note that strerror is allowed to change errno in certain +// circumstances, so we must save errno in lastErrno_ BEFORE we call strerror +// +// Secondly, if errno has not been set at all then we set lastErrno_ = +// ECANCELED; the ECANCELED errno is not used in Pistache code as of Aug/2024. +static const char * strerror_errstr = ""; namespace Pistache { -#define CLIENT_TRY(...) \ - do \ - { \ - auto ret = __VA_ARGS__; \ - if (ret == -1) \ - { \ - lastError_ = strerror(errno); \ - lastErrno_ = errno; \ - return false; \ - } \ +#define CLIENT_TRY(...) \ + do \ + { \ + auto ret = __VA_ARGS__; \ + if (ret == -1) \ + { \ + if (errno) \ + { \ + lastErrno_ = errno; \ + lastError_ = strerror(errno); \ + if (errno != lastErrno_) \ + std::cout << "strerror changed errno (was " << \ + lastErrno_ << ", now " << errno << " )" << std::endl; \ + \ + if (lastError_.empty()) \ + lastError_ = strerror_errstr; \ + } \ + else \ + { \ + std::cout << "ret is -1, but errno not set" << std::endl; \ + if (!lastErrno_) \ + { \ + std::cout << "Setting lastErrno_ to ECANCELED" << \ + std::endl; \ + lastError_ = strerror_errstr; \ + lastErrno_ = ECANCELED; \ + } \ + } \ + return false; \ + } \ } while (0) class TcpClient diff --git a/version.txt b/version.txt index 57a3b2c0f..2774648fd 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.4.2.20240829 +0.4.2.20240830 From 69cc9b4d8261fa582bd99d218a579d5fcbe4c52a Mon Sep 17 00:00:00 2001 From: DMG Date: Thu, 5 Sep 2024 12:35:21 -0700 Subject: [PATCH 2/3] Fix: Check for error being EPIPE only if an error actually occurred --- tests/http_server_test.cc | 19 ++++++++++++------- version.txt | 2 +- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tests/http_server_test.cc b/tests/http_server_test.cc index bbfb984c7..a094b583f 100644 --- a/tests/http_server_test.cc +++ b/tests/http_server_test.cc @@ -986,23 +986,28 @@ TEST(http_server_test, client_request_timeout_on_delay_in_request_line_send_rais const std::string reqStr { "GET /ping HTTP/1.1\r\n" }; TcpClient client; EXPECT_TRUE(client.connect(Pistache::Address("localhost", port))) << client.lastError(); + bool send_failed = false; for (size_t i = 0; i < reqStr.size(); ++i) { if (!client.send(reqStr.substr(i, 1))) { + send_failed = true; break; } std::this_thread::sleep_for(std::chrono::milliseconds(300)); } - EXPECT_EQ(client.lastErrno(), EPIPE) << "Errno: " << client.lastErrno(); + if (send_failed) + { // Usually, send does fail; but on macOS occasionally it does not fail + EXPECT_EQ(client.lastErrno(), EPIPE) << "Errno: " << client.lastErrno(); - char recvBuf[1024] = { - 0, - }; - size_t bytes; - EXPECT_TRUE(client.receive(recvBuf, sizeof(recvBuf), &bytes, std::chrono::seconds(5))) << client.lastError(); - EXPECT_EQ(0, strncmp(recvBuf, ExpectedResponseLine, strlen(ExpectedResponseLine))); + char recvBuf[1024] = { + 0, + }; + size_t bytes; + EXPECT_TRUE(client.receive(recvBuf, sizeof(recvBuf), &bytes, std::chrono::seconds(5))) << client.lastError(); + EXPECT_EQ(0, strncmp(recvBuf, ExpectedResponseLine, strlen(ExpectedResponseLine))); + } server.shutdown(); diff --git a/version.txt b/version.txt index 2774648fd..c9765af0a 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.4.2.20240830 +0.4.2.20240905 From d4523f1e30eb5b9021a9b939a36316947c077b16 Mon Sep 17 00:00:00 2001 From: DMG Date: Fri, 6 Sep 2024 09:49:15 -0700 Subject: [PATCH 3/3] Fix: clang Formatting Conformance --- src/common/reactor.cc | 4 ++-- src/server/listener.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/reactor.cc b/src/common/reactor.cc index 17f763903..c94b39852 100644 --- a/src/common/reactor.cc +++ b/src/common/reactor.cc @@ -129,7 +129,7 @@ namespace Pistache::Aio { std::mutex& poller_reg_unreg_mutex(poller.reg_unreg_mutex_); GUARD_AND_DBG_LOG(poller_reg_unreg_mutex); - + handlers_.forEachHandler([this]( const std::shared_ptr handler) { detachFromReactor(handler); @@ -254,7 +254,7 @@ namespace Pistache::Aio // Note: poller_reg_unreg_mutex is already locked (by // Listener::run()) before calling here, so it is safe to call // handlers_.forEachHandler here - + handlers_.forEachHandler([](const std::shared_ptr handler) { handler->context_.tid = std::this_thread::get_id(); }); diff --git a/src/server/listener.cc b/src/server/listener.cc index f24d88875..61c5e2af2 100644 --- a/src/server/listener.cc +++ b/src/server/listener.cc @@ -385,7 +385,7 @@ namespace Pistache::Tcp F_SETFDL_NOTHING // f_setfl_flags - don't change )); #else - Fd event_fd = actual_fd; + Fd event_fd = actual_fd; #endif LOG_DEBUG_ACT_FD_AND_FDL_FLAGS(actual_fd); @@ -760,7 +760,7 @@ namespace Pistache::Tcp F_SETFDL_NOTHING // f_setfl_flags - don't change )); #else - Fd client_fd = actual_cli_fd; + Fd client_fd = actual_cli_fd; #endif std::shared_ptr peer;