Copilot commented on code in PR #13329:
URL: https://github.com/apache/trafficserver/pull/13329#discussion_r3475858482
##########
src/shared/rpc/IPCSocketClient.cc:
##########
@@ -89,13 +89,24 @@ IPCSocketClient::connect(std::chrono::milliseconds wait_ms,
int attempts)
std::int64_t
IPCSocketClient::_safe_write(int fd, const char *buffer, int len)
{
+ constexpr int WRITE_READY_TIMEOUT_MS = 1000;
+
std::int64_t written{0};
while (written < len) {
const ssize_t ret = ::write(fd, buffer + written, len - written);
if (ret == -1) {
- if (errno == EAGAIN || errno == EINTR) {
+ if (errno == EINTR) {
continue;
}
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ if (write_ready(fd, WRITE_READY_TIMEOUT_MS) == 1) {
+ continue;
+ }
+ }
+ return -1;
Review Comment:
When write readiness polling times out (write_ready returns 0), _safe_write
returns -1 but leaves errno set to EAGAIN/EWOULDBLOCK from the prior write.
That makes the failure look like a transient condition even though this helper
is intentionally failing after a timeout. Consider setting errno (e.g.,
ETIMEDOUT) on write_ready timeout while preserving poll errors (write_ready <
0).
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -344,6 +351,57 @@ send_request(std::string json, std::promise<std::string> p)
auto resp = rpc_client.query(json);
p.set_value(resp);
}
+
+TEST_CASE("IPCSocketClient write returns when the peer stops reading",
"[socket][client]")
+{
+ int fds[2];
+ REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0);
+
+ int const flags = ::fcntl(fds[0], F_GETFL, 0);
+ REQUIRE(flags >= 0);
+ REQUIRE(::fcntl(fds[0], F_SETFL, flags | O_NONBLOCK) == 0);
+
+ std::vector<char> fill(4096, 'x');
+ while (true) {
+ ssize_t const ret = ::write(fds[0], fill.data(), fill.size());
+ if (ret < 0) {
+ REQUIRE((errno == EAGAIN || errno == EWOULDBLOCK));
+ break;
+ }
+ REQUIRE(ret > 0);
+ }
+
+ TestableIPCSocketClient rpc_client;
+ pid_t const pid = ::fork();
+ REQUIRE(pid >= 0);
+ if (pid == 0) {
+ ::close(fds[1]);
+ char const byte = 'x';
+ auto const ret = rpc_client._safe_write(fds[0], &byte, 1);
+ ::close(fds[0]);
+ _exit(ret == -1 ? 0 : 1);
+ }
+
+ int status = 0;
+ for (int i = 0; i < 20; ++i) {
+ if (::waitpid(pid, &status, WNOHANG) == pid) {
+ break;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ }
+
+ if (::waitpid(pid, &status, WNOHANG) == 0) {
+ ::kill(pid, SIGKILL);
+ ::waitpid(pid, &status, 0);
+ FAIL("_safe_write did not return when the nonblocking socket stayed
unwritable");
+ }
Review Comment:
This test uses a fixed ~2s wait for the forked child to exit, but
_safe_write itself may block up to 1s in poll() and can exceed 2s under
scheduler/CI load. Consider using a steady_clock-based deadline with a more
generous bound to reduce flaky timeouts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
