From 817f73e7ae344ca41a2b0782f5a5570e7439a2ed Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 24 Jul 2026 04:44:04 +0800 Subject: [PATCH 01/10] [BUG] Fix nonblocking send/recv handling in the ext/http embedded server The embedded HttpServer mishandled nonblocking send()/recv() results: - sendMore() fell through to sendBuffer.erase(0, sent) when send() returned -1 (EWOULDBLOCK). With sent == -1 the erase count becomes SIZE_MAX and the whole unsent response is wiped. It now never erases on a failed send, and re-registers for writability on a would-block so the buffer is kept. Fixes #4281. - onSocketReadable() closed the connection on any recv() <= 0, including the transient -1/EWOULDBLOCK case. It now closes only on an orderly shutdown (recv() == 0) or a real error, and ignores a transient would-block. Fixes #4282. - Both paths capture the socket error immediately after send()/recv(), before LOG_TRACE can clobber errno / the Winsock last error. EWOULDBLOCK and EAGAIN have the same value on all supported platforms, so a single ErrorWouldBlock check is used (checking both is redundant and trips clang-tidy misc-redundant-expression). Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ .../ext/http/server/http_server.h | 26 ++++++++++++++----- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8baba4503..3ade8620c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ Increment the: namespaces [#4303](https://github.com/open-telemetry/opentelemetry-cpp/pull/4303) +* [BUG] Fix nonblocking send/recv handling in the ext/http embedded server + [#4283](https://github.com/open-telemetry/opentelemetry-cpp/pull/4283) + * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index fed8518e9..891e20238 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -282,15 +282,19 @@ class HttpServer : private SocketTools::Reactor::SocketCallback char buffer[2048] = {0}; int received = socket.recv(buffer, sizeof(buffer)); + int recvError = (received < 0) ? socket.error() : 0; LOG_TRACE("HttpServer: [%s] received %d", conn.request.client.c_str(), received); - if (received <= 0) + if (received > 0) { + conn.receiveBuffer.append(buffer, buffer + received); + handleConnection(conn); + } + else if (received == 0 || recvError != SocketTools::Socket::ErrorWouldBlock) + { + // Orderly shutdown (received == 0) or a real error; a transient would-block is + // retried on the next readable event. handleConnectionClosed(conn); - return; } - conn.receiveBuffer.append(buffer, buffer + received); - - handleConnection(conn); } void onSocketWritable(SocketTools::Socket socket) override @@ -340,12 +344,20 @@ class HttpServer : private SocketTools::Reactor::SocketCallback } int sent = conn.socket.send(conn.sendBuffer.data(), static_cast(conn.sendBuffer.size())); + int sendError = (sent < 0) ? conn.socket.error() : 0; LOG_TRACE("HttpServer: [%s] sent %d", conn.request.client.c_str(), sent); - if (sent < 0 && conn.socket.error() != SocketTools::Socket::ErrorWouldBlock) + if (sent < 0) { + if (sendError == SocketTools::Socket::ErrorWouldBlock) + { + // Backpressure: keep the unsent data and wait for the socket to become writable. + m_reactor.addSocket(conn.socket, + SocketTools::Reactor::Writable | SocketTools::Reactor::Closed); + } + // Never erase the buffer on a failed send: a negative count would wipe it. return true; } - conn.sendBuffer.erase(0, sent); + conn.sendBuffer.erase(0, static_cast(sent)); if (!conn.sendBuffer.empty()) { From 2775214f20c992a5c26985b2a97170cc21e78685 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:50:43 +0800 Subject: [PATCH 02/10] Restore read interest after an asynchronous 100 Continue addSocket() replaces the reactor interest set rather than adding to it, so the would-block path added in this PR leaves a socket registered for writability only. Every other sending state either keeps sending or re-registers Readable once it finishes, but Sending100Continue moved to ReceivingBody without doing so and then returned to wait for a request body it could no longer be woken for. On Linux that is a busy loop on a level-triggered EPOLLOUT with the body never read: an epoll probe registering only EPOLLOUT reports EPOLLOUT on every wait and never EPOLLIN, even with readable bytes already pending. On Windows there is no further FD_WRITE, so the request stalls. The trigger is narrow because it needs a 25 byte interim response to block on a fresh connection, but the interest set is now restored explicitly rather than by luck. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/include/opentelemetry/ext/http/server/http_server.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index 891e20238..43e9a1b7f 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -506,6 +506,13 @@ class HttpServer : private SocketTools::Reactor::SocketCallback return; } + // addSocket() replaces the interest set rather than adding to it, so a would-block + // while sending the interim response leaves the socket registered for writability + // only. Restore read interest before going back to waiting for the request body: + // this is the one transition from a sending state to a receiving one, and the + // keep-alive path at the end of SendingBody already does the same thing. + m_reactor.addSocket(conn.socket, + SocketTools::Reactor::Readable | SocketTools::Reactor::Closed); conn.state = Connection::ReceivingBody; LOG_TRACE("HttpServer: [%s] receiving body", conn.request.client.c_str()); } From 1ae6969ae1c3dc11d18f71061696daf522ca84bb Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:30:09 +0800 Subject: [PATCH 03/10] Narrow the CHANGELOG entry to match the scoped title The PR title was narrowed to "transient would-block handling" but the CHANGELOG line still read "Fix nonblocking send/recv handling", which overstates the scope: fatal-send cleanup, EINTR, Windows short writes, SIGPIPE, and the macOS reactor bugs remain and are tracked in #4287. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ade8620c..d026bb4ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,8 @@ Increment the: namespaces [#4303](https://github.com/open-telemetry/opentelemetry-cpp/pull/4303) -* [BUG] Fix nonblocking send/recv handling in the ext/http embedded server +* [BUG] Fix transient would-block send/recv handling in the ext/http embedded + server [#4283](https://github.com/open-telemetry/opentelemetry-cpp/pull/4283) * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace From 0061a90655d81fdfbb1297a2a1f48701a4816074 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 17:06:56 +0800 Subject: [PATCH 04/10] Add a regression test for the would-block send buffer preservation Fill a nonblocking socket's kernel send buffer, then call sendMore() on a connection with an unsent response so send() returns EWOULDBLOCK. The test asserts the response is kept and sendMore() reports more to send; the old code reached erase(0, sent) with sent == -1, wiping the buffer. Verified fail-before (the pre-fix header fails the assertions) and pass-after. The test reaches the protected sendMore()/Connection through a subclass: under the level-triggered reactor a single send() after a readiness event returns a positive count, so the would-block branch is otherwise only reached from the readable path on a backlogged keep-alive connection, which is hard to stage deterministically end to end. POSIX-only, since it relies on socketpair(). Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/BUILD | 13 +++++ ext/test/http/CMakeLists.txt | 14 +++++ ext/test/http/http_server_test.cc | 85 +++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 ext/test/http/http_server_test.cc diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index 2e6a07e68..3db53373b 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -3,6 +3,19 @@ load("@rules_cc//cc:cc_test.bzl", "cc_test") +cc_test( + name = "http_server_test", + srcs = [ + "http_server_test.cc", + ], + # The would-block test uses a POSIX socketpair; the body is compiled out on Windows. + tags = ["test"], + deps = [ + "//ext:headers", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "curl_http_test", srcs = [ diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index b7707bd8c..c4c3b545e 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -22,3 +22,17 @@ gtest_add_tests( TARGET ${URL_PARSER_FILENAME} TEST_PREFIX ext.http.urlparser. TEST_LIST ${URL_PARSER_FILENAME}) + +# http_server_test drives the would-block path with a POSIX socketpair; the body is compiled out +# on Windows, so build the target only where it has tests (gtest_add_tests parses the source for +# TEST() names and would otherwise register tests the Windows binary does not have). +if(NOT WIN32) + set(HTTP_SERVER_FILENAME http_server_test) + add_executable(${HTTP_SERVER_FILENAME} ${HTTP_SERVER_FILENAME}.cc) + target_link_libraries(${HTTP_SERVER_FILENAME} opentelemetry_ext ${GMOCK_LIB} + ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) + gtest_add_tests( + TARGET ${HTTP_SERVER_FILENAME} + TEST_PREFIX ext.http.httpserver. + TEST_LIST ${HTTP_SERVER_FILENAME}) +endif() diff --git a/ext/test/http/http_server_test.cc b/ext/test/http/http_server_test.cc new file mode 100644 index 000000000..cc6a065e5 --- /dev/null +++ b/ext/test/http/http_server_test.cc @@ -0,0 +1,85 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include + +#include "opentelemetry/ext/http/server/http_server.h" + +// The would-block path is exercised with a POSIX socketpair whose kernel send buffer is filled. +// Windows has no socketpair(); the reactor's would-block handling is identical, so the coverage is +// provided on the POSIX runners. +#ifndef _WIN32 + +# include +# include +# include +# include +# include + +namespace +{ + +// sendMore() is protected because it is a reactor internal. A test subclass reaches it so the +// would-block branch can be driven directly: under the level-triggered reactor a single send() +// after a readiness event returns a positive count, so this branch is otherwise only reached from +// the readable path on a backlogged keep-alive connection, which is hard to stage +// deterministically. +class SendMoreProbe : public HTTP_SERVER_NS::HttpServer +{ +public: + using HttpServer::Connection; + using HttpServer::sendMore; +}; + +// A nonblocking send() to a socket whose kernel send buffer is already full returns -1 with +// EWOULDBLOCK. The old sendMore() fell through to sendBuffer.erase(0, sent) with sent == -1, which +// converts to erase(0, SIZE_MAX) and wipes the entire unsent response. sendMore() must instead keep +// the buffer intact and report that there is more to send. +TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) +{ + int fds[2]; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); + + // A small send buffer plus a nonblocking sender fills quickly; the peer never reads. + int sndbuf = 1024; + ::setsockopt(fds[0], SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf)); + const int fl = ::fcntl(fds[0], F_GETFL, 0); + ASSERT_EQ(::fcntl(fds[0], F_SETFL, fl | O_NONBLOCK), 0); + + // Fill the kernel send buffer so the next send() would block. Bounded so a misbehaving kernel + // cannot spin here forever. + const std::string filler(4096, 'x'); + bool blocked = false; + for (int i = 0; i < 100000; ++i) + { + errno = 0; + const ssize_t sent = ::send(fds[0], filler.data(), filler.size(), MSG_NOSIGNAL); + if (sent < 0 && (errno == EWOULDBLOCK || errno == EAGAIN)) + { + blocked = true; + break; + } + ASSERT_GE(sent, 0); + } + ASSERT_TRUE(blocked) << "could not fill the socket send buffer"; + + SendMoreProbe server; + SendMoreProbe::Connection conn; + conn.socket = SocketTools::Socket(fds[0]); + const std::string response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\nhello"); + conn.sendBuffer = response; + + const bool more = server.sendMore(conn); + + // sendMore() hit the would-block branch: the response is kept, not wiped, and there is more to + // send. The old bug emptied conn.sendBuffer here. + EXPECT_TRUE(more); + EXPECT_EQ(conn.sendBuffer, response); + + conn.socket.close(); // closes fds[0] + ::close(fds[1]); +} + +} // namespace + +#endif // _WIN32 From b0b90512235228200c4b96f368d33698d4bafe65 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:43:20 +0800 Subject: [PATCH 05/10] Fix CI: portable would-block check and send flag in http_server_test GCC maintainer mode (-Wlogical-op -Werror) rejected (errno == EWOULDBLOCK || errno == EAGAIN) because the two values are equal on the supported platforms. Use Socket::ErrorWouldBlock, matching the production code. The buffer-fill send() passed MSG_NOSIGNAL, which is not portable to macOS; the peer stays open so no SIGPIPE is possible. Use flags 0. Add for ssize_t and socket_tools.h for SocketTools::Socket as direct includes for IWYU. Also assert the would-block path re-arms the socket for Writable | Closed, covering the reactor re-registration and not only buffer preservation. --- ext/test/http/http_server_test.cc | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/ext/test/http/http_server_test.cc b/ext/test/http/http_server_test.cc index cc6a065e5..1e21b304a 100644 --- a/ext/test/http/http_server_test.cc +++ b/ext/test/http/http_server_test.cc @@ -4,6 +4,7 @@ #include #include "opentelemetry/ext/http/server/http_server.h" +#include "opentelemetry/ext/http/server/socket_tools.h" // The would-block path is exercised with a POSIX socketpair whose kernel send buffer is filled. // Windows has no socketpair(); the reactor's would-block handling is identical, so the coverage is @@ -13,6 +14,7 @@ # include # include # include +# include # include # include @@ -29,6 +31,21 @@ class SendMoreProbe : public HTTP_SERVER_NS::HttpServer public: using HttpServer::Connection; using HttpServer::sendMore; + + // After a would-block, sendMore() must re-arm the socket for Writable | Closed so the reactor + // calls back once the buffer drains. m_reactor is protected and Reactor::m_sockets is public, so + // the test can read back the software interest flags addSocket() recorded. + int reactorFlags(const SocketTools::Socket &socket) + { + for (const auto &data : m_reactor.m_sockets) + { + if (data.socket == socket) + { + return data.flags; + } + } + return 0; + } }; // A nonblocking send() to a socket whose kernel send buffer is already full returns -1 with @@ -53,8 +70,8 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) for (int i = 0; i < 100000; ++i) { errno = 0; - const ssize_t sent = ::send(fds[0], filler.data(), filler.size(), MSG_NOSIGNAL); - if (sent < 0 && (errno == EWOULDBLOCK || errno == EAGAIN)) + const ssize_t sent = ::send(fds[0], filler.data(), filler.size(), 0); + if (sent < 0 && errno == SocketTools::Socket::ErrorWouldBlock) { blocked = true; break; @@ -75,6 +92,10 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) // send. The old bug emptied conn.sendBuffer here. EXPECT_TRUE(more); EXPECT_EQ(conn.sendBuffer, response); + // The socket is also re-armed for Writable | Closed so the reactor resumes the send once the + // buffer drains; without this the response is kept but the connection would stall forever. + EXPECT_EQ(server.reactorFlags(conn.socket), + SocketTools::Reactor::Writable | SocketTools::Reactor::Closed); conn.socket.close(); // closes fds[0] ::close(fds[1]); From 5b12c003f0a4347b91d98a0f928af3c1bf6de61c Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:00:37 +0800 Subject: [PATCH 06/10] Add deterministic recv-would-block and 100-Continue regression tests HttpServerReadableTest.WouldBlockKeepsTheConnection: a readable event on a nonblocking socket with no data returns EWOULDBLOCK; the connection must stay registered and the socket open, not be torn down as end-of-stream. Driven through the public Reactor callback reference, so no production surface is widened. HttpServer100ContinueTest.CompletedInterimResponseRestoresReadInterest: after the interim 100-Continue response drains, the Sending100Continue -> ReceivingBody transition must restore Readable | Closed; the old code left the socket armed Writable-only so the request body was never read. Both verified fail-before/pass-after against the production fixes. Also assert the setsockopt/fcntl results and skip EINTR in the buffer-fill loop. --- ext/test/http/http_server_test.cc | 86 ++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/ext/test/http/http_server_test.cc b/ext/test/http/http_server_test.cc index 1e21b304a..04c650636 100644 --- a/ext/test/http/http_server_test.cc +++ b/ext/test/http/http_server_test.cc @@ -30,6 +30,9 @@ class SendMoreProbe : public HTTP_SERVER_NS::HttpServer { public: using HttpServer::Connection; + using HttpServer::handleConnection; + using HttpServer::m_connections; + using HttpServer::m_reactor; using HttpServer::sendMore; // After a would-block, sendMore() must re-arm the socket for Writable | Closed so the reactor @@ -59,8 +62,9 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) // A small send buffer plus a nonblocking sender fills quickly; the peer never reads. int sndbuf = 1024; - ::setsockopt(fds[0], SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf)); + ASSERT_EQ(::setsockopt(fds[0], SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf)), 0); const int fl = ::fcntl(fds[0], F_GETFL, 0); + ASSERT_NE(fl, -1); ASSERT_EQ(::fcntl(fds[0], F_SETFL, fl | O_NONBLOCK), 0); // Fill the kernel send buffer so the next send() would block. Bounded so a misbehaving kernel @@ -71,6 +75,10 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) { errno = 0; const ssize_t sent = ::send(fds[0], filler.data(), filler.size(), 0); + if (sent < 0 && errno == EINTR) + { + continue; + } if (sent < 0 && errno == SocketTools::Socket::ErrorWouldBlock) { blocked = true; @@ -101,6 +109,82 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) ::close(fds[1]); } +// A readable event can fire on a nonblocking socket that has no data yet: a spurious wakeup, or a +// peer that connected but has not written. recv() then returns -1 with EWOULDBLOCK. The old +// onSocketReadable() treated every non-positive recv() as end-of-stream and tore the connection +// down; a transient would-block must instead be ignored so the next readable event can retry. +TEST(HttpServerReadableTest, WouldBlockKeepsTheConnection) +{ + int fds[2]; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); + + // The server side is nonblocking; the peer never writes, so recv() reports would-block. + const int fl = ::fcntl(fds[0], F_GETFL, 0); + ASSERT_NE(fl, -1); + ASSERT_EQ(::fcntl(fds[0], F_SETFL, fl | O_NONBLOCK), 0); + + SendMoreProbe server; + const SocketTools::Socket sock(fds[0]); + + // Stage a mid-request connection and arm it exactly as a freshly accepted connection would be. + SendMoreProbe::Connection &conn = server.m_connections[sock]; + conn.socket = sock; + conn.state = SendMoreProbe::Connection::ReceivingHeaders; + server.m_reactor.addSocket(sock, SocketTools::Reactor::Readable | SocketTools::Reactor::Closed); + + // Drive the readable path through the reactor's public callback reference. Virtual dispatch + // reaches the private onSocketReadable() override, so no production surface has to be widened. + server.m_reactor.m_callback.onSocketReadable(sock); + + // The would-block recv() is transient: the connection stays registered and the socket stays open. + // The old bug erased the connection and closed fds[0] here. + EXPECT_TRUE(server.m_connections.find(sock) != server.m_connections.end()); + EXPECT_NE(::fcntl(fds[0], F_GETFD, 0), -1); + + ::close(fds[0]); + ::close(fds[1]); +} + +// After the interim "100 Continue" response has drained, the server must return to reading so it +// can receive the request body. addSocket() replaces (does not merge) the interest set, so a +// would-block during the interim send leaves the socket armed for Writable only. The +// Sending100Continue -> ReceivingBody transition must restore Readable | Closed; the old code +// advanced to ReceivingBody while still armed Writable-only, so the body was never read and the +// connection stalled. +TEST(HttpServer100ContinueTest, CompletedInterimResponseRestoresReadInterest) +{ + int fds[2]; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); + const int fl = ::fcntl(fds[0], F_GETFL, 0); + ASSERT_NE(fl, -1); + ASSERT_EQ(::fcntl(fds[0], F_SETFL, fl | O_NONBLOCK), 0); + + SendMoreProbe server; + SendMoreProbe::Connection conn; + conn.socket = SocketTools::Socket(fds[0]); + // The interim response has finished sending (empty sendBuffer) and a one-byte body is still + // outstanding, so handleConnection() parks in ReceivingBody instead of running the request. + conn.state = SendMoreProbe::Connection::Sending100Continue; + conn.sendBuffer.clear(); + conn.receiveBuffer.clear(); + conn.contentLength = 1; + + // Simulate the Writable-only interest left behind when the interim send blocked. + server.m_reactor.addSocket(conn.socket, + SocketTools::Reactor::Writable | SocketTools::Reactor::Closed); + + server.handleConnection(conn); + + // The interim send is complete, so the server advances to reading the body and restores read + // interest. Without the fix the socket stays armed Writable-only and the body never arrives. + EXPECT_EQ(conn.state, SendMoreProbe::Connection::ReceivingBody); + EXPECT_EQ(server.reactorFlags(conn.socket), + SocketTools::Reactor::Readable | SocketTools::Reactor::Closed); + + ::close(fds[0]); + ::close(fds[1]); +} + } // namespace #endif // _WIN32 From 458ddcef4f957fedbadc1a213a86367868449510 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:22:37 +0800 Subject: [PATCH 07/10] Fix Format CI: cmake-format ext/test/http/CMakeLists.txt The http_server_test registration comment was not wrapped to the cmake-format line width; apply cmake-format with the repo config. --- ext/test/http/CMakeLists.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index c4c3b545e..e6f4597ae 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -23,9 +23,10 @@ gtest_add_tests( TEST_PREFIX ext.http.urlparser. TEST_LIST ${URL_PARSER_FILENAME}) -# http_server_test drives the would-block path with a POSIX socketpair; the body is compiled out -# on Windows, so build the target only where it has tests (gtest_add_tests parses the source for -# TEST() names and would otherwise register tests the Windows binary does not have). +# http_server_test drives the would-block path with a POSIX socketpair; the body +# is compiled out on Windows, so build the target only where it has tests +# (gtest_add_tests parses the source for TEST() names and would otherwise +# register tests the Windows binary does not have). if(NOT WIN32) set(HTTP_SERVER_FILENAME http_server_test) add_executable(${HTTP_SERVER_FILENAME} ${HTTP_SERVER_FILENAME}.cc) From e86987ff2cbe2eb2616ae5974b442ca662240bf0 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:45:49 +0800 Subject: [PATCH 08/10] Rewrite test comments to describe behavior, not code history Per maintainer guidance, code comments describe what the code does and why, not what a previous revision did. State the required invariant and failure mode rather than 'the old code did X'. --- ext/test/http/http_server_test.cc | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/ext/test/http/http_server_test.cc b/ext/test/http/http_server_test.cc index 04c650636..0c680b2a7 100644 --- a/ext/test/http/http_server_test.cc +++ b/ext/test/http/http_server_test.cc @@ -52,9 +52,9 @@ class SendMoreProbe : public HTTP_SERVER_NS::HttpServer }; // A nonblocking send() to a socket whose kernel send buffer is already full returns -1 with -// EWOULDBLOCK. The old sendMore() fell through to sendBuffer.erase(0, sent) with sent == -1, which -// converts to erase(0, SIZE_MAX) and wipes the entire unsent response. sendMore() must instead keep -// the buffer intact and report that there is more to send. +// EWOULDBLOCK. sendMore() must treat that as transient: keep the unsent response intact and report +// there is more to send. Erasing on a negative count would compute erase(0, SIZE_MAX) and wipe the +// entire response. TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) { int fds[2]; @@ -96,8 +96,8 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) const bool more = server.sendMore(conn); - // sendMore() hit the would-block branch: the response is kept, not wiped, and there is more to - // send. The old bug emptied conn.sendBuffer here. + // sendMore() hit the would-block branch: the response is kept (not wiped) and there is more to + // send. EXPECT_TRUE(more); EXPECT_EQ(conn.sendBuffer, response); // The socket is also re-armed for Writable | Closed so the reactor resumes the send once the @@ -110,9 +110,9 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) } // A readable event can fire on a nonblocking socket that has no data yet: a spurious wakeup, or a -// peer that connected but has not written. recv() then returns -1 with EWOULDBLOCK. The old -// onSocketReadable() treated every non-positive recv() as end-of-stream and tore the connection -// down; a transient would-block must instead be ignored so the next readable event can retry. +// peer that connected but has not written. recv() then returns -1 with EWOULDBLOCK, which +// onSocketReadable() must treat as transient (ignore it and wait for the next readable event) +// rather than as end-of-stream that tears the connection down. TEST(HttpServerReadableTest, WouldBlockKeepsTheConnection) { int fds[2]; @@ -136,8 +136,7 @@ TEST(HttpServerReadableTest, WouldBlockKeepsTheConnection) // reaches the private onSocketReadable() override, so no production surface has to be widened. server.m_reactor.m_callback.onSocketReadable(sock); - // The would-block recv() is transient: the connection stays registered and the socket stays open. - // The old bug erased the connection and closed fds[0] here. + // The would-block recv() is transient: the connection must stay registered and the socket open. EXPECT_TRUE(server.m_connections.find(sock) != server.m_connections.end()); EXPECT_NE(::fcntl(fds[0], F_GETFD, 0), -1); @@ -147,10 +146,9 @@ TEST(HttpServerReadableTest, WouldBlockKeepsTheConnection) // After the interim "100 Continue" response has drained, the server must return to reading so it // can receive the request body. addSocket() replaces (does not merge) the interest set, so a -// would-block during the interim send leaves the socket armed for Writable only. The -// Sending100Continue -> ReceivingBody transition must restore Readable | Closed; the old code -// advanced to ReceivingBody while still armed Writable-only, so the body was never read and the -// connection stalled. +// would-block during the interim send leaves the socket armed for Writable only; the +// Sending100Continue -> ReceivingBody transition must therefore restore Readable | Closed, +// otherwise the body is never read and the connection stalls. TEST(HttpServer100ContinueTest, CompletedInterimResponseRestoresReadInterest) { int fds[2]; @@ -176,7 +174,7 @@ TEST(HttpServer100ContinueTest, CompletedInterimResponseRestoresReadInterest) server.handleConnection(conn); // The interim send is complete, so the server advances to reading the body and restores read - // interest. Without the fix the socket stays armed Writable-only and the body never arrives. + // interest. EXPECT_EQ(conn.state, SendMoreProbe::Connection::ReceivingBody); EXPECT_EQ(server.reactorFlags(conn.socket), SocketTools::Reactor::Readable | SocketTools::Reactor::Closed); From 153870d94e8cbb21cd024b787dfbd6fb345822fb Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:57:01 +0800 Subject: [PATCH 09/10] Fix IWYU: include and in http_server_test reactorFlags() iterates m_reactor.m_sockets (std::vector) and the readable-path test uses m_connections (std::map), so IWYU (warning_limit 0) requires both headers directly. --- ext/test/http/http_server_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ext/test/http/http_server_test.cc b/ext/test/http/http_server_test.cc index 0c680b2a7..c61a81ee5 100644 --- a/ext/test/http/http_server_test.cc +++ b/ext/test/http/http_server_test.cc @@ -16,7 +16,9 @@ # include # include # include +# include # include +# include namespace { From cc75666b75283984aa5312e2ae39c517aaa31e9a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:09:57 +0800 Subject: [PATCH 10/10] Assert reactor registration survives a recv would-block; tighten test wording WouldBlockKeepsTheConnection also asserts reactorFlags(sock) == Readable | Closed: a transient recv would-block must leave the socket's reactor registration intact, not merely keep the connection in the map with an open fd. Also make three comments precise: stale readiness (not an idle peer) as the would-block cause, the shared HttpServer decision logic (not the whole reactor) as what is platform-independent, and 'not expected on the normal Linux path' rather than 'not reachable'. --- ext/test/http/http_server_test.cc | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/ext/test/http/http_server_test.cc b/ext/test/http/http_server_test.cc index c61a81ee5..e5455e5b7 100644 --- a/ext/test/http/http_server_test.cc +++ b/ext/test/http/http_server_test.cc @@ -7,8 +7,9 @@ #include "opentelemetry/ext/http/server/socket_tools.h" // The would-block path is exercised with a POSIX socketpair whose kernel send buffer is filled. -// Windows has no socketpair(); the reactor's would-block handling is identical, so the coverage is -// provided on the POSIX runners. +// Windows has no socketpair(); the shared HttpServer would-block decision logic these tests drive +// is platform-independent, so the coverage is provided on the POSIX runners. Windows-specific +// reactor notification behavior is outside this target. #ifndef _WIN32 # include @@ -25,9 +26,9 @@ namespace // sendMore() is protected because it is a reactor internal. A test subclass reaches it so the // would-block branch can be driven directly: under the level-triggered reactor a single send() -// after a readiness event returns a positive count, so this branch is otherwise only reached from -// the readable path on a backlogged keep-alive connection, which is hard to stage -// deterministically. +// after a writable readiness event returns a positive count on the normal Linux path, so this +// branch is otherwise reached from the readable path on a backlogged keep-alive connection, which +// is hard to stage deterministically. class SendMoreProbe : public HTTP_SERVER_NS::HttpServer { public: @@ -111,10 +112,10 @@ TEST(HttpServerSendMoreTest, WouldBlockPreservesTheSendBuffer) ::close(fds[1]); } -// A readable event can fire on a nonblocking socket that has no data yet: a spurious wakeup, or a -// peer that connected but has not written. recv() then returns -1 with EWOULDBLOCK, which -// onSocketReadable() must treat as transient (ignore it and wait for the next readable event) -// rather than as end-of-stream that tears the connection down. +// A readiness notification can go stale before the callback runs, so a nonblocking recv() in +// onSocketReadable() can return -1 with EWOULDBLOCK even though the readable callback fired. That +// transient must be ignored (wait for the next readable event) rather than treated as end-of-stream +// that tears the connection down. TEST(HttpServerReadableTest, WouldBlockKeepsTheConnection) { int fds[2]; @@ -138,9 +139,13 @@ TEST(HttpServerReadableTest, WouldBlockKeepsTheConnection) // reaches the private onSocketReadable() override, so no production surface has to be widened. server.m_reactor.m_callback.onSocketReadable(sock); - // The would-block recv() is transient: the connection must stay registered and the socket open. + // The would-block recv() is transient: the connection stays in the map, the socket stays open, + // and its reactor registration is untouched (still Readable | Closed) so a later readable event + // can retry. EXPECT_TRUE(server.m_connections.find(sock) != server.m_connections.end()); EXPECT_NE(::fcntl(fds[0], F_GETFD, 0), -1); + EXPECT_EQ(server.reactorFlags(sock), + SocketTools::Reactor::Readable | SocketTools::Reactor::Closed); ::close(fds[0]); ::close(fds[1]);