diff --git a/CHANGELOG.md b/CHANGELOG.md index 6abdc6117..64361e018 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,63 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `main`, the release pipeline automatically replaces `[current]` with the next version number before tagging the release. +## [current] + +### Changed + +- **`std.http.client` resolves the backend host only when it is about to dial** + (#1719). `getaddrinfo` ran unconditionally above the pool lookup, so every + request resolved the host and then discarded the answer on a pooled hit — a + lock-taking call per request for a result nobody used. Resolution now happens + behind a once-flag at the four dial sites (the initial dial plus three + pooled-connection retry paths). Worth **+0.56%** on the LB benchmark (47,887 → + 48,156 rps); the gain is small here because the benchmark's backends are + numeric IPs, which `getaddrinfo` short-circuits — against named upstreams, + where resolution can touch `/etc/hosts` or the network, it removes real work. + One deliberate behaviour change: a request hitting a live pooled connection + now succeeds even if the host has since stopped resolving, rather than failing + with "could not resolve host". An open connection does not need DNS. + +- **`std.http.client`'s idle connection pool no longer walks its whole list on + every request** (#1719). `http_pool_take` and `http_pool_put` each swept the + idle list under the global pool mutex per request; with the default 15s idle + window and a proxy reusing upstreams continuously, that sweep frees nothing + almost every time. The pool now tracks when its earliest connection becomes + eligible and skips the walk until then. Separately, the per-key cap check + stops counting once it reaches the cap instead of walking to the end. **No + measurable throughput change** (48,854 → 48,870 rps over three alternating + A/B rounds, baseline ahead in two of them) — kept because it is strictly less + work under a global mutex, which matters with more upstreams than the + two-backend benchmark has, not as a performance claim. + +- **`std.http.server` and `std.http.client` no longer re-apply socket timeouts + that are already set** (#1719). Under `strace` against the load-balancer + benchmark, `setsockopt` was the third costliest syscall — 202,552 calls for + 20,000 requests, ~10 per request, 15.2% of syscall time — and not one of them + changed a socket option. Two sites, both on the keep-alive path: every reuse + of a pooled upstream connection re-applied `SO_RCVTIMEO`/`SO_SNDTIMEO`, and + every request on a parked client connection re-applied the idle timeout. Both + now remember the value they applied and skip the call when it is unchanged; + the unguarded form stays on the dial path, where the socket is new. Measured + effect: `setsockopt` falls from 202,552 calls to **72**, total syscalls per + request from **83.0 to 14.0**, and throughput rises 2.6% (47,852 → 49,083 rps, + three alternating A/B rounds, new ahead in every round). Note the parking + path's comment already claimed the window was "only re-applied when it + changes" — that guard did not exist until now. + +- **`std.http.server` resolves a connection's peer and local address once per + connection rather than once per request** (#1719). The old comment reasoned + that `getpeername`/`getsockname` are cache-warm and therefore cheap enough to + run per request; measured against nginx on the same box, that was 2 syscalls, + 2 `inet_ntop` calls and 2 `strdup`s on every request, where nginx makes none + of them — neither address can change while a socket is open. Caching them on + `HttpConn` (which connection parking, #1663, had already made + connection-lifetime) took the load-balancer benchmark from 48,073 to 49,432 + rps, **+2.8%**, with nginx and haproxy measured in the same runs as controls + moving under 0.3%. Under `strace`, `getpeername` disappears from the profile + and `getsockname` falls from 133,162 calls to 50. The request still owns its + own copies, so `http_request_free`'s contract is unchanged. + ## [0.577.0] ### Fixed diff --git a/std/net/aether_http.c b/std/net/aether_http.c index d8e9710cc..b475c969d 100644 --- a/std/net/aether_http.c +++ b/std/net/aether_http.c @@ -44,6 +44,7 @@ const char* http_response_read_chunk_raw(HttpResponse* r, int max) { (void)r; (v #include #include "../../runtime/utils/aether_thread.h" #include +#include /* INT64_MAX, for the pool expiry watermark */ #ifdef _WIN32 #include @@ -269,6 +270,20 @@ static char* ssl_err_string(const char* prefix) { typedef struct { int sockfd; + /* The SO_RCVTIMEO/SO_SNDTIMEO value currently on this socket, or -1 when + * nothing has been applied yet (#1719). + * + * A Transport travels with its connection into the idle pool, so a reused + * connection already carries the timeouts the last request set. Re-applying + * an identical value costs 2 setsockopt syscalls per request and changes + * nothing: under strace against the LB benchmark, setsockopt was the third + * costliest syscall at 202,552 calls for 20,000 requests -- roughly 10 per + * request, on sockets whose options were already correct. + * + * A sentinel of -1 rather than 0 because 0 is a legitimate timeout value + * meaning "block indefinitely", and a socket set to block forever must not + * be confused with one never configured. */ + int64_t applied_timeout_ns; #ifdef AETHER_HAS_OPENSSL SSL* ssl; /* A per-request SSL_CTX, owned by this transport, or NULL when the @@ -398,6 +413,22 @@ static int http_pool_enabled = 1; static int http_pool_max_idle = 64; static int http_pool_max_per_key = 8; static int64_t http_pool_idle_ms = 15000; +/* When the oldest pooled connection becomes eligible for expiry, or INT64_MAX + * when the pool is empty (#1719). + * + * http_pool_take and http_pool_put both swept the whole idle list on every + * request, under the global pool mutex. With the default 15s idle window and a + * proxy reusing its upstreams continuously, that sweep frees nothing on the + * overwhelming majority of calls -- it is a list walk per request to discover + * there is nothing to do. Tracking the earliest deadline lets both callers skip + * the walk outright until that time actually arrives. + * + * Kept deliberately coarse: it is a lower bound, not an exact answer. Entries + * are only ever added with a fresh idle_since_ms, so the earliest deadline can + * only move later when the list shrinks, and recomputing it during a sweep we + * were doing anyway is free. A stale-early value costs one redundant sweep, + * never a missed expiry. */ +static int64_t http_pool_next_expiry_ms = INT64_MAX; static int64_t http_now_ms(void) { struct timespec ts; @@ -421,8 +452,15 @@ static void http_pool_key(char* out, size_t n, const char* host, int port, use_tls ? 1 : 0, insecure ? 1 : 0, cafile ? cafile : ""); } -/* Caller holds the lock. Drops every entry idle past the timeout. */ +/* Caller holds the lock. Drops every entry idle past the timeout. + * + * Returns immediately when the earliest deadline is still in the future, which + * is the common case on a busy proxy -- see http_pool_next_expiry_ms. When it + * does sweep, it recomputes that watermark from the survivors. */ static void http_pool_expire_locked(int64_t now) { + if (now < http_pool_next_expiry_ms) return; + + int64_t earliest = INT64_MAX; HttpIdleConn** link = &http_pool_head; while (*link) { HttpIdleConn* c = *link; @@ -433,8 +471,11 @@ static void http_pool_expire_locked(int64_t now) { free(c); continue; } + int64_t due = c->idle_since_ms + http_pool_idle_ms; + if (due < earliest) earliest = due; link = &c->next; } + http_pool_next_expiry_ms = earliest; } /* Take an idle connection for `key`, newest first (the most recently used is @@ -476,9 +517,15 @@ static void http_pool_put(const char* key, Transport* t) { int64_t now = http_now_ms(); pthread_mutex_lock(http_pool_lock()); http_pool_expire_locked(now); + /* Only whether the per-key cap is REACHED matters, not the exact count, so + * stop at the cap instead of walking to the end -- and skip the walk + * entirely when the global cap already rejects this connection. */ int per_key = 0; - for (HttpIdleConn* e = http_pool_head; e; e = e->next) { - if (strcmp(e->key, key) == 0) per_key++; + if (http_pool_count < http_pool_max_idle) { + for (HttpIdleConn* e = http_pool_head; e; e = e->next) { + if (strcmp(e->key, key) == 0 && ++per_key >= http_pool_max_per_key) + break; + } } if (http_pool_count >= http_pool_max_idle || per_key >= http_pool_max_per_key) { pthread_mutex_unlock(http_pool_lock()); @@ -492,6 +539,10 @@ static void http_pool_put(const char* key, Transport* t) { c->next = http_pool_head; http_pool_head = c; http_pool_count++; + /* On an empty pool the watermark is INT64_MAX, and this entry is now the + * only deadline there is. Lowering it here is what re-arms the sweep. */ + if (c->idle_since_ms + http_pool_idle_ms < http_pool_next_expiry_ms) + http_pool_next_expiry_ms = c->idle_since_ms + http_pool_idle_ms; pthread_mutex_unlock(http_pool_lock()); t->sockfd = -1; #ifdef AETHER_HAS_OPENSSL @@ -506,6 +557,7 @@ void http_client_pool_clear_raw(void) { HttpIdleConn* c = http_pool_head; http_pool_head = NULL; http_pool_count = 0; + http_pool_next_expiry_ms = INT64_MAX; /* nothing left to expire */ pthread_mutex_unlock(http_pool_lock()); while (c) { HttpIdleConn* next = c->next; @@ -528,6 +580,11 @@ const char* http_client_pool_configure_raw(int max_idle, int max_per_host, if (idle_ns >= 0) { int64_t ms = idle_ns / 1000000LL; http_pool_idle_ms = ms > 0 ? ms : 1; + /* The expiry watermark was derived from the PREVIOUS window, so + * shortening the window would leave a deadline further out than the new + * setting allows and delay every eviction. Force the next pool + * operation to sweep and recompute it. */ + http_pool_next_expiry_ms = INT64_MIN; } pthread_mutex_unlock(http_pool_lock()); if (!http_pool_enabled) http_client_pool_clear_raw(); @@ -1210,6 +1267,19 @@ static void http_apply_timeouts(int sockfd, int64_t timeout_ns) { #endif } +/* Apply timeouts only when they differ from what the socket already carries. + * + * The unguarded http_apply_timeouts stays for the dial path, where the socket + * is new and its option state is genuinely unknown. This form is for the reuse + * path, where the answer is usually "already correct" -- see the comment on + * Transport::applied_timeout_ns for the measurement. */ +static void transport_apply_timeouts(Transport* t, int64_t timeout_ns) { + int64_t want = timeout_ns < 0 ? 0 : timeout_ns; + if (t->applied_timeout_ns == want) return; + http_apply_timeouts(t->sockfd, want); + t->applied_timeout_ns = want; +} + /* Is a pooled connection still usable? A peer that closed leaves the socket * readable at EOF, and anything readable on an idle keep-alive connection is * unexpected in either direction (a stray byte would desynchronise the next @@ -1238,6 +1308,45 @@ static int transport_is_live(Transport* t) { return ready == 0; } +/* Resolve `host`:`port` into `out`, at most once per request (#1719). + * + * `*resolved` is the caller's once-flag: 0 on entry means "not yet", and it is + * set on success so a second call is free. Returns 1 on success (including the + * cached case), 0 when the name does not resolve. + * + * Resolve via getaddrinfo, NOT gethostbyname: gethostbyname returns a pointer + * into a shared, process-static `struct hostent`, so two client calls resolving + * at once on different threads -- e.g. a request handler that dials out while + * serving (serve-and-dial), where the inner call runs on a server worker thread + * -- race on that static buffer and can corrupt each other's resolved address. + * getaddrinfo is thread-safe and returns caller-owned memory freed with + * freeaddrinfo. Pinned to AF_INET: the callers build a sockaddr_in and the + * timeout/connect path assumes IPv4, so widening to IPv6 is a separate change. + * + * A failed resolve leaves *resolved at 0, so a later caller retries rather than + * reading an uninitialised address. */ +static int resolve_dial_addr(const char* host, int port, + struct sockaddr_in* out, int* resolved) { + if (*resolved) return 1; + + char port_str[16]; + snprintf(port_str, sizeof(port_str), "%d", port); + struct addrinfo hints; + memset(&hints, 0, sizeof(hints)); + hints.ai_family = AF_INET; + hints.ai_socktype = SOCK_STREAM; + struct addrinfo* res = NULL; + if (getaddrinfo(host, port_str, &hints, &res) != 0 || !res) return 0; + + memset(out, 0, sizeof(*out)); + memcpy(out, res->ai_addr, sizeof(struct sockaddr_in)); + freeaddrinfo(res); + out->sin_family = AF_INET; + out->sin_port = htons(port); + *resolved = 1; + return 1; +} + /* Set *err to a heap copy of `msg`, for the dial helper's error returns. */ static void ae_set_err(char** out_err, const char* msg) { if (!out_err) return; @@ -1391,6 +1500,9 @@ static int http_dial(HttpClientRequest* req, struct sockaddr_in* serv_addr_in, } out->sockfd = sockfd; + /* The dial path below applies the timeouts unconditionally on a fresh + * socket; record the value so a later reuse can skip re-applying it. */ + out->applied_timeout_ns = req->timeout_ns < 0 ? 0 : req->timeout_ns; /* HTTPS via a forward proxy: establish a CONNECT tunnel over the raw socket * BEFORE the TLS handshake, so TLS runs end-to-end through the proxy (the @@ -1615,38 +1727,34 @@ static HttpResponse* http_request_internal(HttpClientRequest* req) { const char* dial_host = connect_host; int dial_port = connect_port; - /* Resolve via getaddrinfo, NOT gethostbyname: gethostbyname returns a - * pointer into a shared, process-static `struct hostent`, so two client - * calls resolving at once on different threads, e.g. a request handler - * that dials out while serving (serve-and-dial), where the inner call runs - * on a server worker thread, race on that static buffer and can corrupt - * each other's resolved address. getaddrinfo is thread-safe and returns - * caller-owned memory freed with freeaddrinfo. Pinned to AF_INET: the rest - * of this function builds a sockaddr_in and the timeout/connect path - * assumes IPv4, so widening to IPv6 is a separate change. */ + /* The dial address, resolved lazily (#1719). + * + * Resolution used to run unconditionally, above the pool lookup, so every + * request resolved the backend host and threw the answer away on a pooled + * hit -- a lock-taking call per request for a result nobody used. Only the + * two http_dial sites consume it, and the pool key is built from + * dial_host/dial_port rather than the resolved address, so nothing before + * a dial needs it. + * + * Both dial sites go through resolve_dial_addr, which resolves at most + * once per request: the send-failure retry below re-dials a connection + * that came from the pool, and that is precisely the path where the first + * resolve was skipped. + * + * One deliberate behaviour change falls out of this: a request that hits a + * live pooled connection now succeeds even if the host has since stopped + * resolving, where before it failed with "could not resolve host". An open + * connection does not need DNS, and holding an established socket hostage + * to a resolver blip is the less useful behaviour -- it is also what nginx + * does. A request that has to dial still fails exactly as it did. */ struct sockaddr_in serv_addr; - memset(&serv_addr, 0, sizeof(serv_addr)); - { - char port_str[16]; - snprintf(port_str, sizeof(port_str), "%d", dial_port); - struct addrinfo hints; - memset(&hints, 0, sizeof(hints)); - hints.ai_family = AF_INET; - hints.ai_socktype = SOCK_STREAM; - struct addrinfo* res = NULL; - int gai = getaddrinfo(dial_host, port_str, &hints, &res); - if (gai != 0 || !res) { - response->error = string_new(via_proxy ? "could not resolve proxy host" - : "could not resolve host"); - return response; - } - memcpy(&serv_addr, res->ai_addr, sizeof(struct sockaddr_in)); - freeaddrinfo(res); - } - serv_addr.sin_family = AF_INET; - serv_addr.sin_port = htons(dial_port); + int serv_addr_resolved = 0; - Transport t; + /* Zero-initialised so applied_timeout_ns starts at the "nothing applied" + * sentinel rather than stack garbage, which the reuse guard would read as + * a real value and wrongly skip the setsockopt. */ + Transport t = {0}; + t.applied_timeout_ns = -1; char pool_key[HTTP_POOL_KEY_MAX]; http_pool_key(pool_key, sizeof(pool_key), host, port, use_tls, dial_host, dial_port, req->insecure, req->cafile); @@ -1660,12 +1768,19 @@ static HttpResponse* http_request_internal(HttpClientRequest* req) { if (pool_this && http_pool_take(pool_key, &t)) { if (transport_is_live(&t)) { reused = 1; - http_apply_timeouts(t.sockfd, req->timeout_ns); + transport_apply_timeouts(&t, req->timeout_ns); } else { transport_close(&t); } } if (!reused) { + if (!resolve_dial_addr(dial_host, dial_port, &serv_addr, + &serv_addr_resolved)) { + response->error = string_new(via_proxy ? "could not resolve proxy host" + : "could not resolve host"); + return response; + } + char* dial_err = NULL; if (http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &dial_err) != 0) { response->error = string_new(dial_err ? dial_err : "connection failed"); @@ -1798,7 +1913,11 @@ static HttpResponse* http_request_internal(HttpClientRequest* req) { reused = 0; transport_close(&t); char* rd_err = NULL; - if (http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &rd_err) == 0) { + /* This connection came from the pool, so the resolve above was + * skipped; do it now. A failure here just means no retry. */ + if (resolve_dial_addr(dial_host, dial_port, &serv_addr, + &serv_addr_resolved) + && http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &rd_err) == 0) { free(rd_err); goto send_request; } @@ -1818,7 +1937,10 @@ static HttpResponse* http_request_internal(HttpClientRequest* req) { reused = 0; transport_close(&t); char* rd_err = NULL; - if (http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &rd_err) == 0) { + /* Pooled connection, so the resolve was skipped; do it now. */ + if (resolve_dial_addr(dial_host, dial_port, &serv_addr, + &serv_addr_resolved) + && http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &rd_err) == 0) { free(rd_err); goto send_request; } @@ -2009,7 +2131,10 @@ static HttpResponse* http_request_internal(HttpClientRequest* req) { reused = 0; transport_close(&t); char* rd_err = NULL; - if (http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &rd_err) == 0) { + /* Pooled connection, so the resolve was skipped; do it now. */ + if (resolve_dial_addr(dial_host, dial_port, &serv_addr, + &serv_addr_resolved) + && http_dial(req, &serv_addr, host, port, use_tls, via_proxy, &t, &rd_err) == 0) { free(rd_err); aether_caps_free(full_response, cap); full_response = NULL; diff --git a/std/net/aether_http_server.c b/std/net/aether_http_server.c index b886373bd..3372b424a 100644 --- a/std/net/aether_http_server.c +++ b/std/net/aether_http_server.c @@ -231,6 +231,43 @@ typedef struct HttpConn { * back to a different worker, so the count has to live with the * connection rather than on the worker's stack (#1663). */ int requests_served; + /* Peer and local address, resolved once per CONNECTION (#1719). + * + * These used to be fetched per request, on the reasoning that + * getpeername/getsockname are cache-warm and therefore cheap. Measured + * against nginx on the same box, that cost 2 syscalls, 2 inet_ntop calls + * and 2 strdups on every request — nginx makes zero of any of them, + * because neither address can change while a connection is open. + * + * Cached as text because that is the shape every consumer wants + * (http_request_remote_addr returns a string). Empty string means the + * lookup failed — a Unix-domain socket, or an fd that closed under us — + * and is a valid cached answer, so `addrs_resolved` distinguishes + * "looked and found nothing" from "not looked yet". Without that flag a + * failing lookup would retry on every request, which is the cost this + * removes. + * + * The request still owns its own copies: http_request_free frees + * req->remote_addr and req->local_addr, so per-request strdups from + * these are what get handed out. That keeps the ownership contract + * unchanged; the saving is the syscalls and the inet_ntop, not the + * allocation. */ + char remote_addr[INET6_ADDRSTRLEN]; + char local_addr[INET6_ADDRSTRLEN]; + int remote_port; + int local_port; + int addrs_resolved; + + /* The SO_RCVTIMEO value currently on this socket, or -1 when none has been + * applied yet (#1719). + * + * conn_serve applies the idle timeout on entry, and with connection parking + * a kept-alive connection re-enters conn_serve once per request -- so an + * unguarded apply is one setsockopt per request setting the value that is + * already there. The comment on the parking path already claimed the window + * was "only re-applied when it changes"; this is the guard that makes that + * true. -1 rather than 0 because 0 is a valid "block indefinitely". */ + int applied_recv_timeout_ms; } HttpConn; /* The parking lot holds HttpConn by pointer and needs exactly two things from @@ -2791,6 +2828,60 @@ static int conn_recv_timed_out(void) { #endif } +/* Resolve this connection's peer and local address, once (#1719). + * + * Idempotent: after the first call `addrs_resolved` is set and the cached + * text is returned unchanged, including when the lookup failed and the + * cache holds "". A failed lookup is a real answer for a Unix-domain + * socket, and retrying it per request is exactly the cost being removed. + * + * Not thread-safe by design: an HttpConn is owned by one worker at a + * time. Parking hands it between workers, but never concurrently. */ +static void conn_resolve_addrs(HttpConn* c) { + if (!c || c->addrs_resolved) return; + c->addrs_resolved = 1; + c->remote_addr[0] = '\0'; + c->local_addr[0] = '\0'; + c->remote_port = 0; + c->local_port = 0; + if (c->fd < 0) return; + + struct sockaddr_storage ss; + socklen_t sslen = sizeof(ss); + if (getpeername(c->fd, (struct sockaddr*)&ss, &sslen) == 0) { + const char* src = NULL; + if (ss.ss_family == AF_INET) { + struct sockaddr_in* sa = (struct sockaddr_in*)&ss; + src = inet_ntop(AF_INET, &sa->sin_addr, + c->remote_addr, sizeof(c->remote_addr)); + c->remote_port = ntohs(sa->sin_port); + } else if (ss.ss_family == AF_INET6) { + struct sockaddr_in6* sa = (struct sockaddr_in6*)&ss; + src = inet_ntop(AF_INET6, &sa->sin6_addr, + c->remote_addr, sizeof(c->remote_addr)); + c->remote_port = ntohs(sa->sin6_port); + } + if (!src) { c->remote_addr[0] = '\0'; c->remote_port = 0; } + } + + sslen = sizeof(ss); + if (getsockname(c->fd, (struct sockaddr*)&ss, &sslen) == 0) { + const char* src = NULL; + if (ss.ss_family == AF_INET) { + struct sockaddr_in* sa = (struct sockaddr_in*)&ss; + src = inet_ntop(AF_INET, &sa->sin_addr, + c->local_addr, sizeof(c->local_addr)); + c->local_port = ntohs(sa->sin_port); + } else if (ss.ss_family == AF_INET6) { + struct sockaddr_in6* sa = (struct sockaddr_in6*)&ss; + src = inet_ntop(AF_INET6, &sa->sin6_addr, + c->local_addr, sizeof(c->local_addr)); + c->local_port = ntohs(sa->sin6_port); + } + if (!src) { c->local_addr[0] = '\0'; c->local_port = 0; } + } +} + static int handle_one_request(HttpServer* server, HttpConn* conn, int requests_served, int max_requests) { long t_start = http_now_us(); @@ -2958,64 +3049,40 @@ static int handle_one_request(HttpServer* server, HttpConn* conn, req->body_length = (size_t)content_length; } - /* Populate the connection-level metadata that handlers learn from - * the kernel rather than from the request bytes. Cheap (two - * cache-warm syscalls + two inet_ntop's) so it runs per request; - * failures (Unix-domain socket, EBADF on a just-closed fd) leave - * each field at its zero default and the accessors return ""/0. + + /* Connection-level metadata that handlers learn from the kernel rather + * than from the request bytes. + * + * Resolved ONCE per connection (#1719) — neither address can change + * while the socket is open, and fetching them per request cost 2 + * syscalls, 2 inet_ntop calls and 2 strdups each time. nginx makes none + * of those calls at all. + * + * - remote_addr/remote_port (getpeername): the trusted peer. The + * X-Forwarded-For header is client-supplied and a wrong basis for an + * allow/deny decision on a direct listener; this is the kernel's view + * of the socket, which is unspoofable. + * - local_addr/local_port (getsockname): which NIC this accepted fd is + * bound to. Needed when the listener binds 0.0.0.0 and the handler + * gates on the receiving interface (admin-on-loopback, multi-tenant + * per-IP routing). + * - is_tls: the connection wrapper, not anything in the wire bytes. + * Drives scheme/redirect/cookie-Secure decisions. * - * - remote_addr/remote_port (from getpeername): the trusted peer. - * The X-Forwarded-For header is client-supplied and a wrong - * basis for an allow/deny decision on a direct listener; this - * is the kernel's view of the socket, which is unspoofable. - * - local_addr/local_port (from getsockname): which NIC this - * accepted fd is bound to. Needed when the listener binds - * 0.0.0.0 and the handler wants to gate behaviour on which - * interface received the request (admin-on-loopback, multi- - * tenant per-IP routing). - * - is_tls: the connection wrapper, not anything in the wire - * bytes. Drives scheme/redirect/cookie-Secure decisions. + * The request keeps owning its copies — http_request_free frees both — + * so the strdups stay and only the syscalls go. A failed lookup leaves + * the cache empty and the accessors return ""/0, as before. * - * IPv4 + IPv6 supported via sockaddr_storage. */ + * IPv4 + IPv6 via sockaddr_storage. */ { - struct sockaddr_storage ss; - socklen_t sslen = sizeof(ss); - if (getpeername(conn->fd, (struct sockaddr*)&ss, &sslen) == 0) { - char buf[INET6_ADDRSTRLEN]; - const char* src = NULL; - int port = 0; - if (ss.ss_family == AF_INET) { - struct sockaddr_in* sa = (struct sockaddr_in*)&ss; - src = inet_ntop(AF_INET, &sa->sin_addr, buf, sizeof(buf)); - port = ntohs(sa->sin_port); - } else if (ss.ss_family == AF_INET6) { - struct sockaddr_in6* sa = (struct sockaddr_in6*)&ss; - src = inet_ntop(AF_INET6, &sa->sin6_addr, buf, sizeof(buf)); - port = ntohs(sa->sin6_port); - } - if (src) { - req->remote_addr = strdup(src); - req->remote_port = port; - } + conn_resolve_addrs(conn); + if (conn->remote_addr[0]) { + req->remote_addr = strdup(conn->remote_addr); + req->remote_port = conn->remote_port; } - sslen = sizeof(ss); - if (getsockname(conn->fd, (struct sockaddr*)&ss, &sslen) == 0) { - char buf[INET6_ADDRSTRLEN]; - const char* src = NULL; - int port = 0; - if (ss.ss_family == AF_INET) { - struct sockaddr_in* sa = (struct sockaddr_in*)&ss; - src = inet_ntop(AF_INET, &sa->sin_addr, buf, sizeof(buf)); - port = ntohs(sa->sin_port); - } else if (ss.ss_family == AF_INET6) { - struct sockaddr_in6* sa = (struct sockaddr_in6*)&ss; - src = inet_ntop(AF_INET6, &sa->sin6_addr, buf, sizeof(buf)); - port = ntohs(sa->sin6_port); - } - if (src) { - req->local_addr = strdup(src); - req->local_port = port; - } + if (conn->local_addr[0]) { + req->local_addr = strdup(conn->local_addr); + req->local_port = conn->local_port; } req->is_tls = (conn->ssl != NULL) ? 1 : 0; } @@ -3537,9 +3604,12 @@ static int64_t conn_now_ms(void) { return (int64_t)time(NULL) * 1000; } -static void conn_apply_recv_timeout(HttpServer* server, int fd) { +static void conn_apply_recv_timeout(HttpServer* server, HttpConn* c) { + int fd = c->fd; int idle_ms = server->keep_alive_idle_ms > 0 ? server->keep_alive_idle_ms : 30000; + if (c->applied_recv_timeout_ms == idle_ms) return; + c->applied_recv_timeout_ms = idle_ms; #ifdef _WIN32 DWORD rcv_timeout = (DWORD)idle_ms; setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, @@ -3596,7 +3666,7 @@ static void conn_serve(HttpServer* server, HttpConn* conn) { * The wait is a plain blocking recv either way: a poll before it would be * a syscall per request, and measured at 8 concurrent clients that cost * 9% against simply blocking. */ - conn_apply_recv_timeout(server, conn->fd); + conn_apply_recv_timeout(server, conn); #ifdef AETHER_HAS_NGHTTP2 if (conn->is_h2) { @@ -3697,6 +3767,9 @@ void http_server_drain_connection(HttpServer* server, int client_fd) { conn->read_pos = 0; conn->write_pos = 0; conn->requests_served = 0; + /* calloc would leave this 0, which is a legitimate timeout meaning "block + * indefinitely"; the guard must see "nothing applied yet" instead. */ + conn->applied_recv_timeout_ms = -1; #ifdef AETHER_HAS_OPENSSL if (server->tls_enabled && server->tls_ctx) { diff --git a/tests/integration/http_request_conn_accessors/server.ae b/tests/integration/http_request_conn_accessors/server.ae index a5d3d6f17..1517380bf 100644 --- a/tests/integration/http_request_conn_accessors/server.ae +++ b/tests/integration/http_request_conn_accessors/server.ae @@ -49,6 +49,17 @@ main() { println("READY") + // Keep-alive on, so the multi-request half of the shell test actually + // reuses one connection. Without it curl reconnects per request, the + // per-connection address cache (#1719) is repopulated each time, and + // the assertion that cached values survive a reused connection would + // pass without testing anything. + ka_err = http.server_set_keepalive(raw, 1, 0, 10000ms) + if ka_err != "" { + println("FAIL: server_set_keepalive: ${ka_err}") + exit(1) + } + http.server_get(raw, "/whoami", handle_whoami, 0) spawn(SchedHelper()) diff --git a/tests/integration/http_request_conn_accessors/test_http_request_conn_accessors.sh b/tests/integration/http_request_conn_accessors/test_http_request_conn_accessors.sh index 7604237e9..fbaf46995 100755 --- a/tests/integration/http_request_conn_accessors/test_http_request_conn_accessors.sh +++ b/tests/integration/http_request_conn_accessors/test_http_request_conn_accessors.sh @@ -74,4 +74,37 @@ VER=$(get_field ver) [ "$IS_TLS" = "0" ] || { echo " [FAIL] is_tls expected 0 (cleartext listener), got '$IS_TLS'"; echo " raw: $RESP"; exit 1; } [ "$VER" = "HTTP/1.1" ] || { echo " [FAIL] ver expected HTTP/1.1, got '$VER'"; echo " raw: $RESP"; exit 1; } -echo " [PASS] http_request_conn_accessors (remote_port + local_addr/port + scheme + is_tls + http_version)" +# The addresses are resolved once per CONNECTION and cached (#1719), not +# fetched per request. That is only correct if every request on a reused +# connection still reports them — a cache populated for request 1 and not +# read back for request 2 would leave the later ones empty, which no +# single-request check would notice. +# +# curl's multi-URL form reuses one connection, so this drives three +# requests down one socket and requires all three to agree. The peer PORT +# is the sharpest field here: it is kernel-assigned per connection, so if +# the cache were somehow refreshed mid-connection it would still match, +# but if it were dropped the field would go empty. +MULTI=$(curl --silent --show-error --max-time 5 "$URL" "$URL" "$URL" 2>"$TMPDIR/m.err") || { + echo " [FAIL] keep-alive curl failed:"; cat "$TMPDIR/m.err"; exit 1 +} + +# The three bodies concatenate with no separator, so a line-oriented +# count would see "ver=HTTP/1.1peer=127.0.0.1" as one field. Count +# occurrences instead. +# `wc -l` pads its count with leading blanks on BSD/macOS (" 3"), +# so a string compare against "3" passes on GNU and fails on Darwin -- +# which is exactly how this test went red on both macOS legs and green +# everywhere else. Strip the padding rather than compare numerically, so +# the failure message still prints a clean count. +MULTI_PEERS=$(echo "$MULTI" | grep -o 'peer=127\.0\.0\.1' | wc -l | tr -d '[:space:]') +[ "$MULTI_PEERS" = "3" ] || { + echo " [FAIL] expected peer=127.0.0.1 on all 3 keep-alive requests, got $MULTI_PEERS" + echo " raw: $MULTI"; exit 1; } + +MULTI_LOCALS=$(echo "$MULTI" | grep -o 'local_port=18294' | wc -l | tr -d '[:space:]') +[ "$MULTI_LOCALS" = "3" ] || { + echo " [FAIL] expected local_port=18294 on all 3 keep-alive requests, got $MULTI_LOCALS" + echo " raw: $MULTI"; exit 1; } + +echo " [PASS] http_request_conn_accessors (remote_port + local_addr/port + scheme + is_tls + http_version; cached addrs survive keep-alive)"