diff --git a/README.md b/README.md index fca3fc4d..94bb47b9 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ See [`docs/PLAN.md`](docs/PLAN.md) for the full phased plan and | 4 | Server generation (restJson1 + rpcv2Cbor) | ✅ Done — handlers, routing, serde, all HTTP bindings incl. `@httpPayload`/`@httpPrefixHeaders`, constraint validation, parser strictness, content negotiation; ~1,175 official conformance cases green ([docs/server-guide.md](docs/server-guide.md)) | | 5 | Generated-client ↔ generated-server integration harness | ✅ Done — every fixture ships a generated integration suite: seeded random round-trips over loopback and real sockets, per-error mapping, unknown-member tolerance, mutation-checked ([docs/design/integration-testing.md](docs/design/integration-testing.md)) | | 6 | Bazel rules, CLI, packaging (BCR + Maven Central), docs site | 🔨 In progress — `smithy_cpp_{types,client,server}_library` rules run the generator hermetically inside the build graph, out-of-tree consumer module tested in CI, CLI via `bazel run //codegen:generator` ([docs/quickstart.md](docs/quickstart.md)); BCR/Maven publishing deferred until production validation; docs site pending | -| 7 | Hardening, fuzzing, v0.1.0 | 🔨 In progress — retries with full-jitter exponential backoff, gzip `@requestCompression` (client + server), consumer CI across linux/macos/windows ([docs/production-guide.md](docs/production-guide.md)) | +| 7 | Hardening, fuzzing, v0.1.0 | 🔨 In progress — retries with full-jitter exponential backoff, gzip `@requestCompression` (client + server), client interceptors + server middleware (auth/logging/metrics seams), Beast graceful drain + header limits, consumer CI across linux/macos/windows ([docs/production-guide.md](docs/production-guide.md)) | | 8 | Bidirectional streaming (event streams, WebSockets) | Not started | ## Building diff --git a/codegen/smithy-cpp-codegen/src/main/java/io/smithycpp/codegen/ClientGenerator.java b/codegen/smithy-cpp-codegen/src/main/java/io/smithycpp/codegen/ClientGenerator.java index aacc9725..1f75d965 100644 --- a/codegen/smithy-cpp-codegen/src/main/java/io/smithycpp/codegen/ClientGenerator.java +++ b/codegen/smithy-cpp-codegen/src/main/java/io/smithycpp/codegen/ClientGenerator.java @@ -158,7 +158,9 @@ private void writeSource(CppWriter w) { w.openBlock("if (!request.body.empty()) {"); w.write("request.headers.Set(\"content-length\", std::to_string(request.body.size()));"); w.closeBlock("}"); - w.write("return smithy::SendWithRetries(*transport_, request, config_.retry);"); + w.write( + "return smithy::SendWithRetries(*transport_, request, config_.retry, " + + "config_.interceptors);"); w.closeBlock("}"); w.write(""); diff --git a/docs/PLAN.md b/docs/PLAN.md index 0b4b4887..efe6953b 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -525,6 +525,15 @@ the project) completes the tutorial without help; BCR + Maven Central packaging malformed tests. - **Server robustness**: thread-pool tuning knobs, graceful shutdown/drain, request size limits, slow-client timeouts, structured logging hooks, metrics hooks (request count/latency callbacks). +- **User-supplied middleware**: first-class extension seams on both sides, so auth, logging, + tracing, and metrics are user-composable rather than one-off knobs (smithy-rs prior art: + client interceptors + tower layers). Client side: an interceptor chain on `ClientConfig` with + hooks around the full call and around each attempt (mutate the outgoing `HttpRequest` — + e.g. inject auth headers — and observe the `HttpResponse`/outcome). Server side: middleware + wrapping the transport-facing `RequestHandler` (a decorator: pre-dispatch request + inspection/rejection, post-dispatch response observation), composing outside the generated + router so it works with any transport. The logging/metrics hooks above and the auth hooks + below should be built as middleware on these seams, not as parallel mechanisms. - **Auth hooks**: `@httpBearerAuth` / `@httpApiKeyAuth` support — client-side credential providers, server-side authenticator interface (vendor-specific schemes such as SigV4 are out of scope, per §2). diff --git a/docs/production-guide.md b/docs/production-guide.md index dc80a81d..ed920ae2 100644 --- a/docs/production-guide.md +++ b/docs/production-guide.md @@ -86,10 +86,70 @@ Compression trades CPU for bytes: leave the 10 KiB threshold alone unless you have measured small-payload wins; compressing tiny bodies usually inflates them. +## Client interceptors + +`config.interceptors` (`smithy/client/interceptor.h`) hooks user code around +every HTTP attempt a generated client makes — auth headers, tracing ids, +request/response logging — without touching generated code: + +```cpp +class BearerAuth final : public smithy::Interceptor { + public: + void ModifyBeforeTransmit(smithy::http::HttpRequest& request, int attempt) override { + request.headers.Set("authorization", "Bearer " + LoadToken()); + } + void ReadAfterTransmit(const smithy::http::HttpRequest& request, + const smithy::Outcome& outcome, + int attempt) override { + LogAttempt(request.target, attempt, outcome.ok() ? outcome->status : -1); + } +}; + +config.interceptors.push_back(std::make_shared()); +``` + +Interceptors run in registration order, around each attempt (retries included +— `attempt` is 1-based). `ModifyBeforeTransmit` mutates a fresh copy of the +request per attempt, so edits never accumulate across retries or leak into +the caller's view. Hooks must not throw. + +## Server middleware + +Generated servers expose their router as a plain +`smithy::http::RequestHandler`, so cross-cutting server behavior — auth +checks, request logging, metrics — composes as middleware outside the +generated code (`smithy/server/middleware.h`), with any transport: + +```cpp +WeatherServer server(handler); + +auto require_auth = [](smithy::http::RequestHandler next) { + return [next = std::move(next)](const smithy::http::HttpRequest& request) { + if (!Authorized(request)) return smithy::http::HttpResponse{401, {}, ""}; + return next(request); + }; +}; + +transport.Start(smithy::server::Chain( + {require_auth, + smithy::server::Observe([](const smithy::server::RequestObservation& o) { + // o.method, o.target, o.status, o.duration — log or feed metrics + // (count = one callback per request, latency = o.duration). + })}, + server.Handler())); +``` + +The first middleware in the chain is outermost: it sees the request first and +the response last, and can short-circuit before the router runs. `Observe` is +the built-in structured-logging/metrics hook; its callback runs on the +transport's request thread, so keep it cheap or hand off. + ## Server hardening -The production server transport (`BeastServerTransport`, ADR-0006) already -enforces per-connection timeouts, body-size limits, and graceful shutdown; -see [server-guide.md](server-guide.md). Phase 7b extends this area -(thread-pool sizing, drain, slow-client handling, logging/metrics hooks) — -see [PLAN.md](PLAN.md). +The production server transport (`BeastServerTransport`, ADR-0006) enforces +per-connection timeouts (`request_timeout_seconds`), body- and header-size +limits (`max_body_bytes`, `max_header_bytes`), and drains on `Stop()`: new +connections and keep-alive reads cease immediately, while requests already +read off the wire get up to `drain_timeout_seconds` (default 10) to finish +writing their responses before the thread pool is torn down. See +[server-guide.md](server-guide.md). diff --git a/docs/runtime.md b/docs/runtime.md index 05c44ed1..fec0dbae 100644 --- a/docs/runtime.md +++ b/docs/runtime.md @@ -11,10 +11,10 @@ crates (PLAN §3.2a). | `//runtime:json` | `smithy::json` | `Document` ⇄ JSON text via nlohmann (blobs as base64, timestamps per stored format) | | `//runtime:cbor` | `smithy::cbor` | `Document` ⇄ deterministic CBOR (RFC 8949; tag-1 timestamps; tolerant decoder) — ADR-0005 | | `//runtime:http` | `smithy::http` | `Headers` (case-insensitive), URI percent-encoding per the Smithy HTTP binding rules, `HttpRequest`/`HttpResponse`, `HttpClient`/`HttpServerTransport` interfaces, `Loopback` in-memory transport, built-in `SocketHttpClient`/`SocketHttpServer` (test/reference only — ADR-0006) | -| `//runtime:http_beast` | `smithy::http` | `BeastServerTransport` (ADR-0006): the production server transport on BCR modular Boost.Beast/asio — concurrent connections on a thread pool, keep-alive, per-connection timeouts, body-size limits, graceful shutdown. Separate target so Boost stays out of dep-light builds | -| `//runtime:client` | `smithy` | `ClientConfig` (endpoint, timeout, user-agent, transport injection, `RetryPolicy`, request-compression threshold), `SendWithRetries` (full-jitter exponential backoff over transport errors and 429/5xx — see docs/production-guide.md) | +| `//runtime:http_beast` | `smithy::http` | `BeastServerTransport` (ADR-0006): the production server transport on BCR modular Boost.Beast/asio — concurrent connections on a thread pool, keep-alive, per-connection timeouts, body- and header-size limits, graceful drain on Stop. Separate target so Boost stays out of dep-light builds | +| `//runtime:client` | `smithy` | `ClientConfig` (endpoint, timeout, user-agent, transport injection, `RetryPolicy`, request-compression threshold, `Interceptor` hooks around every attempt), `SendWithRetries` (full-jitter exponential backoff over transport errors and 429/5xx — see docs/production-guide.md) | | `//runtime:compression` | `smithy` | `GzipCompress`/`GzipDecompress` (zlib; decompression-bomb guard, trailing-garbage rejection) backing `@requestCompression` | -| `//runtime:server` | `smithy::server` | `Router` (literal > label > greedy precedence, 404/405/400), `RequestContext`, `MakeErrorResponse`, `ValidationFailure` | +| `//runtime:server` | `smithy::server` | `Router` (literal > label > greedy precedence, 404/405/400), `RequestContext`, `MakeErrorResponse`, `ValidationFailure`, user-supplied `Middleware` + `Chain` + the `Observe` logging/metrics hook | ## Design rules diff --git a/docs/server-guide.md b/docs/server-guide.md index fc1b6f3c..0536f3d7 100644 --- a/docs/server-guide.md +++ b/docs/server-guide.md @@ -47,6 +47,11 @@ transport.Start(server.Handler()); // or smithy::http::Loopback for in-process tests, SocketHttpServer for the built-in listener. ``` +Cross-cutting behavior (auth checks, request logging, metrics) wraps `server.Handler()` as +user-supplied middleware — `smithy::server::Chain` composes it outside the generated router, +and `smithy::server::Observe` is the built-in logging/metrics hook. See +[production-guide.md](production-guide.md). + Routing (method + URI pattern from `@http`, greedy labels, 404/405 with `Allow`), request-binding deserialization (labels, query incl. `@httpQueryParams`, headers, JSON/CBOR bodies), and response serialization (status, headers, body) are all generated; rpcv2Cbor diff --git a/examples/cafe/generated/src/client.cc b/examples/cafe/generated/src/client.cc index 5292c2d2..4e0a8504 100644 --- a/examples/cafe/generated/src/client.cc +++ b/examples/cafe/generated/src/client.cc @@ -121,7 +121,7 @@ smithy::Outcome CafeClient::Send(smithy::http::HttpR if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome CafeClient::GetOrder(const GetOrderInput& input) const { diff --git a/examples/roundtrip/rest/generated/src/client.cc b/examples/roundtrip/rest/generated/src/client.cc index 7bd222bd..11b3255c 100644 --- a/examples/roundtrip/rest/generated/src/client.cc +++ b/examples/roundtrip/rest/generated/src/client.cc @@ -174,7 +174,7 @@ smithy::Outcome RoundTripRestClient::Send(smithy::ht if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome RoundTripRestClient::DescribeSink(const DescribeSinkInput& input) const { diff --git a/examples/roundtrip/rpc/generated/src/client.cc b/examples/roundtrip/rpc/generated/src/client.cc index 55364937..e973b959 100644 --- a/examples/roundtrip/rpc/generated/src/client.cc +++ b/examples/roundtrip/rpc/generated/src/client.cc @@ -116,7 +116,7 @@ smithy::Outcome RoundTripRpcClient::Send(smithy::htt if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome RoundTripRpcClient::PutSinkRpc(const PutSinkRpcInput& input) const { diff --git a/examples/weather/generated/src/client.cc b/examples/weather/generated/src/client.cc index 17747b2c..985732af 100644 --- a/examples/weather/generated/src/client.cc +++ b/examples/weather/generated/src/client.cc @@ -158,7 +158,7 @@ smithy::Outcome WeatherClient::Send(smithy::http::Ht if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome WeatherClient::DeleteCity(const DeleteCityInput& input) const { diff --git a/examples/weather/generated_server_e2e_test.cc b/examples/weather/generated_server_e2e_test.cc index 64506811..1aaa2398 100644 --- a/examples/weather/generated_server_e2e_test.cc +++ b/examples/weather/generated_server_e2e_test.cc @@ -6,12 +6,16 @@ #include #include +#include +#include #include "example/weather/client.h" #include "example/weather/server.h" #include "examples/weather/handwritten/weather_client.h" +#include "smithy/client/interceptor.h" #include "smithy/http/loopback.h" #include "smithy/http/message.h" +#include "smithy/server/middleware.h" namespace example::weather { namespace { @@ -121,6 +125,65 @@ TEST_F(GeneratedServerEndToEndTest, GeneratedClientRetriesTransientFailures) { EXPECT_EQ(city->name, "Seattle"); } +// Client interceptor + server middleware together (Phase 7b): the interceptor +// injects a bearer token on every generated-client request; server middleware +// rejects requests without it before the router runs, and Observe reports the +// served request. +TEST_F(GeneratedServerEndToEndTest, InterceptorAndMiddlewareCarryAuthAcrossTheWire) { + class BearerAuth final : public smithy::Interceptor { + public: + void ModifyBeforeTransmit(smithy::http::HttpRequest& request, int) override { + request.headers.Set("authorization", "Bearer smoke-token"); + } + }; + + std::vector observations; + auto require_auth = [](smithy::http::RequestHandler next) { + return [next = std::move(next)](const smithy::http::HttpRequest& request) { + if (request.headers.Get("authorization") != "Bearer smoke-token") { + smithy::http::HttpResponse response; + response.status = 401; + return response; + } + return next(request); + }; + }; + auto handler = smithy::server::Chain( + {require_auth, smithy::server::Observe([&](const smithy::server::RequestObservation& o) { + observations.push_back(o); + })}, + server_->Handler()); + + auto loopback = std::make_shared(); + ASSERT_TRUE(loopback->Start(handler).ok()); + + // Without the interceptor the middleware rejects the call outright. + { + smithy::ClientConfig config; + config.http_client = loopback; + config.retry.max_attempts = 1; + auto client = example::weather::WeatherClient::Create(std::move(config)); + ASSERT_TRUE(client.ok()); + const auto city = client->GetCity(example::weather::GetCityInput{.cityId = "seattle"}); + ASSERT_FALSE(city.ok()); + } + + smithy::ClientConfig config; + config.http_client = loopback; + config.interceptors.push_back(std::make_shared()); + auto client = example::weather::WeatherClient::Create(std::move(config)); + ASSERT_TRUE(client.ok()); + const auto city = client->GetCity(example::weather::GetCityInput{.cityId = "seattle"}); + ASSERT_TRUE(city.ok()) << city.error().message(); + EXPECT_EQ(city->name, "Seattle"); + + // Observe sits inside the auth check, so only the authorized call reports. + ASSERT_EQ(observations.size(), 1u); + EXPECT_EQ(observations[0].method, "GET"); + EXPECT_EQ(observations[0].target, "/cities/seattle"); + EXPECT_EQ(observations[0].status, 200); +} + TEST_F(GeneratedServerEndToEndTest, DeleteCityIs204WithNoBody) { smithy::http::HttpRequest request; request.method = "DELETE"; diff --git a/protocol-tests/restjson1-validation/generated/src/client.cc b/protocol-tests/restjson1-validation/generated/src/client.cc index af38597c..d8675c41 100644 --- a/protocol-tests/restjson1-validation/generated/src/client.cc +++ b/protocol-tests/restjson1-validation/generated/src/client.cc @@ -206,7 +206,7 @@ smithy::Outcome RestJsonValidationClient::Send(smith if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome RestJsonValidationClient::MalformedEnum(const MalformedEnumInput& input) const { diff --git a/protocol-tests/restjson1/generated/src/client.cc b/protocol-tests/restjson1/generated/src/client.cc index 75d22b1b..468881ed 100644 --- a/protocol-tests/restjson1/generated/src/client.cc +++ b/protocol-tests/restjson1/generated/src/client.cc @@ -177,7 +177,7 @@ smithy::Outcome RestJsonClient::Send(smithy::http::H if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome RestJsonClient::AllQueryStringTypes(const AllQueryStringTypesInput& input) const { diff --git a/protocol-tests/rpcv2cbor/generated/src/client.cc b/protocol-tests/rpcv2cbor/generated/src/client.cc index 50b112aa..762f45a5 100644 --- a/protocol-tests/rpcv2cbor/generated/src/client.cc +++ b/protocol-tests/rpcv2cbor/generated/src/client.cc @@ -152,7 +152,7 @@ smithy::Outcome RpcV2ProtocolClient::Send(smithy::ht if (!request.body.empty()) { request.headers.Set("content-length", std::to_string(request.body.size())); } - return smithy::SendWithRetries(*transport_, request, config_.retry); + return smithy::SendWithRetries(*transport_, request, config_.retry, config_.interceptors); } smithy::Outcome RpcV2ProtocolClient::EmptyInputOutput(const EmptyInputOutputInput& input) const { diff --git a/runtime/BUILD.bazel b/runtime/BUILD.bazel index 415516f5..e75ee915 100644 --- a/runtime/BUILD.bazel +++ b/runtime/BUILD.bazel @@ -201,6 +201,7 @@ cc_library( srcs = ["src/client/retry.cc"], hdrs = [ "include/smithy/client/config.h", + "include/smithy/client/interceptor.h", "include/smithy/client/retry.h", ], copts = COPTS, @@ -224,8 +225,14 @@ cc_test( cc_library( name = "server", - srcs = ["src/server/router.cc"], - hdrs = ["include/smithy/server/router.h"], + srcs = [ + "src/server/middleware.cc", + "src/server/router.cc", + ], + hdrs = [ + "include/smithy/server/middleware.h", + "include/smithy/server/router.h", + ], copts = COPTS, includes = ["include"], deps = [ @@ -245,6 +252,17 @@ cc_test( ], ) +cc_test( + name = "middleware_test", + size = "small", + srcs = ["tests/server/middleware_test.cc"], + copts = COPTS, + deps = [ + ":server", + "@googletest//:gtest_main", + ], +) + # Test-only helpers (random Document generator for property tests). cc_library( name = "testing", diff --git a/runtime/include/smithy/client/config.h b/runtime/include/smithy/client/config.h index 34052c4b..f1289ed2 100644 --- a/runtime/include/smithy/client/config.h +++ b/runtime/include/smithy/client/config.h @@ -3,7 +3,9 @@ #include #include +#include +#include "smithy/client/interceptor.h" #include "smithy/client/retry.h" #include "smithy/http/transport.h" @@ -30,6 +32,10 @@ struct ClientConfig { // (the Smithy default; 0 compresses everything). int request_min_compression_size_bytes = 10240; + // User-supplied hooks around every HTTP attempt (auth headers, logging, + // tracing); run in registration order. See smithy/client/interceptor.h. + std::vector> interceptors; + // Optional transport override; shared so several clients can reuse one. std::shared_ptr http_client; }; diff --git a/runtime/include/smithy/client/interceptor.h b/runtime/include/smithy/client/interceptor.h new file mode 100644 index 00000000..e81d8e8f --- /dev/null +++ b/runtime/include/smithy/client/interceptor.h @@ -0,0 +1,38 @@ +#ifndef SMITHY_CLIENT_INTERCEPTOR_H_ +#define SMITHY_CLIENT_INTERCEPTOR_H_ + +#include "smithy/core/outcome.h" +#include "smithy/http/message.h" + +namespace smithy { + +// User-supplied hooks around every HTTP attempt a generated client makes +// (smithy-rs prior art: client interceptors). Register on +// ClientConfig::interceptors; interceptors run in registration order. +// Hooks must not throw — express failures by leaving the request unusable +// for the server (e.g. dropping credentials) rather than raising. +class Interceptor { + public: + virtual ~Interceptor() = default; + + // Runs before each attempt (attempt is 1-based; retries see 2, 3, ...). + // Mutate the outgoing request here: auth headers, tracing ids, ... + virtual void ModifyBeforeTransmit(http::HttpRequest& request, int attempt) { + (void)request; + (void)attempt; + } + + // Runs after each attempt with the request as sent and the transport + // outcome (a response of any status, or a transport error). Observe only: + // logging, metrics, tracing. + virtual void ReadAfterTransmit(const http::HttpRequest& request, + const Outcome& outcome, int attempt) { + (void)request; + (void)outcome; + (void)attempt; + } +}; + +} // namespace smithy + +#endif // SMITHY_CLIENT_INTERCEPTOR_H_ diff --git a/runtime/include/smithy/client/retry.h b/runtime/include/smithy/client/retry.h index 33237fed..41be564f 100644 --- a/runtime/include/smithy/client/retry.h +++ b/runtime/include/smithy/client/retry.h @@ -3,7 +3,10 @@ #include #include +#include +#include +#include "smithy/client/interceptor.h" #include "smithy/core/outcome.h" #include "smithy/http/message.h" #include "smithy/http/transport.h" @@ -34,10 +37,18 @@ bool RetryableStatus(int status); // Sends through the transport with retries: transport failures flagged // retryable (connection, timeout) and transient response statuses are // retried up to policy.max_attempts, sleeping the backoff in between. -// The last outcome — success or not — is returned as-is. -Outcome SendWithRetries(http::HttpClient& transport, - const http::HttpRequest& request, - const RetryPolicy& policy); +// The last outcome — success or not — is returned as-is. Interceptors run +// around every attempt (ModifyBeforeTransmit on a per-attempt copy of the +// request, ReadAfterTransmit on the outcome), in registration order. +Outcome SendWithRetries( + http::HttpClient& transport, const http::HttpRequest& request, const RetryPolicy& policy, + const std::vector>& interceptors); + +inline Outcome SendWithRetries(http::HttpClient& transport, + const http::HttpRequest& request, + const RetryPolicy& policy) { + return SendWithRetries(transport, request, policy, {}); +} } // namespace smithy diff --git a/runtime/include/smithy/http/beast_transport.h b/runtime/include/smithy/http/beast_transport.h index 0361efce..c739fd81 100644 --- a/runtime/include/smithy/http/beast_transport.h +++ b/runtime/include/smithy/http/beast_transport.h @@ -28,6 +28,10 @@ class BeastServerTransport : public HttpServerTransport { int threads = 4; int request_timeout_seconds = 30; std::size_t max_body_bytes = std::size_t{64} * 1024 * 1024; + std::size_t max_header_bytes = std::size_t{8} * 1024; + // Stop() drains: no new connections or keep-alive reads, and in-flight + // requests get this long to finish before the pool is torn down. + int drain_timeout_seconds = 10; }; BeastServerTransport() : BeastServerTransport(Options{}) {} diff --git a/runtime/include/smithy/server/middleware.h b/runtime/include/smithy/server/middleware.h new file mode 100644 index 00000000..b2c9eee2 --- /dev/null +++ b/runtime/include/smithy/server/middleware.h @@ -0,0 +1,45 @@ +#ifndef SMITHY_SERVER_MIDDLEWARE_H_ +#define SMITHY_SERVER_MIDDLEWARE_H_ + +#include +#include +#include +#include + +#include "smithy/http/transport.h" + +namespace smithy::server { + +// User-supplied middleware wraps the transport-facing handler a generated +// server exposes: inspect or reject requests before dispatch, observe or +// amend responses after. Middleware composes outside the generated router, +// so it works with any HttpServerTransport: +// +// WeatherServer server(handler); +// transport.Start(smithy::server::Chain({AuthCheck(), Observe(log)}, +// server.Handler())); +using Middleware = std::function; + +// Composes middleware around a handler. The first element is outermost: it +// sees the request first and the response last. +http::RequestHandler Chain(std::vector middleware, http::RequestHandler handler); + +// One served request, as seen from outside the router. +struct RequestObservation { + std::string method; + std::string target; + int status = 0; + std::chrono::milliseconds duration{0}; +}; + +// Middleware reporting every request to a callback — the structured-logging +// and metrics hook (count = callbacks, latency = duration). The callback runs +// on the transport's request thread after the response is built; keep it +// cheap or hand off. now is injectable for deterministic tests (null means +// steady_clock). +Middleware Observe(std::function callback, + std::function now = nullptr); + +} // namespace smithy::server + +#endif // SMITHY_SERVER_MIDDLEWARE_H_ diff --git a/runtime/src/client/retry.cc b/runtime/src/client/retry.cc index a3dce58e..35222792 100644 --- a/runtime/src/client/retry.cc +++ b/runtime/src/client/retry.cc @@ -28,16 +28,33 @@ bool RetryableStatus(int status) { return status == 429 || status == 500 || status == 502 || status == 503 || status == 504; } -Outcome SendWithRetries(http::HttpClient& transport, - const http::HttpRequest& request, - const RetryPolicy& policy) { +Outcome SendWithRetries( + http::HttpClient& transport, const http::HttpRequest& request, const RetryPolicy& policy, + const std::vector>& interceptors) { const auto sleep = policy.sleep != nullptr ? policy.sleep : [](std::chrono::milliseconds d) { std::this_thread::sleep_for(d); }; const auto jitter = policy.jitter != nullptr ? policy.jitter : UniformJitter; const int attempts = std::max(policy.max_attempts, 1); - Outcome outcome = transport.Send(request); + // Each attempt mutates a fresh copy, so interceptor edits never accumulate + // across retries. + const auto attempt_send = [&](int attempt) -> Outcome { + if (interceptors.empty()) { + return transport.Send(request); // skip the request copy + } + http::HttpRequest attempt_request = request; + for (const auto& interceptor : interceptors) { + interceptor->ModifyBeforeTransmit(attempt_request, attempt); + } + Outcome outcome = transport.Send(attempt_request); + for (const auto& interceptor : interceptors) { + interceptor->ReadAfterTransmit(attempt_request, outcome, attempt); + } + return outcome; + }; + + Outcome outcome = attempt_send(1); for (int retry = 1; retry < attempts; ++retry) { const bool retryable = outcome.ok() ? RetryableStatus(outcome->status) : outcome.error().retryable(); @@ -45,7 +62,7 @@ Outcome SendWithRetries(http::HttpClient& transport, return outcome; } sleep(RetryDelay(policy, retry, jitter())); - outcome = transport.Send(request); + outcome = attempt_send(retry + 1); } return outcome; } diff --git a/runtime/src/http/beast_transport.cc b/runtime/src/http/beast_transport.cc index 88cc7d9b..f06f5dcb 100644 --- a/runtime/src/http/beast_transport.cc +++ b/runtime/src/http/beast_transport.cc @@ -1,12 +1,16 @@ #include "smithy/http/beast_transport.h" +#include #include #include +#include #include #include #include #include +#include #include +#include #include namespace smithy::http { @@ -53,6 +57,9 @@ struct BeastServerTransport::State : std::enable_shared_from_this { Options opts; RequestHandler handler; std::atomic stopping{false}; + // Requests read off the wire whose responses are not fully written yet; + // Stop() drains until this reaches zero (or the drain deadline passes). + std::atomic active{0}; // All completion handlers capture State weakly: handlers queued inside the // io_context must not own the State that owns the io_context, or abandoned @@ -80,6 +87,7 @@ struct BeastServerTransport::State : std::enable_shared_from_this { auto buffer = std::make_shared(); auto parser = std::make_shared>(); parser->body_limit(opts.max_body_bytes); + parser->header_limit(static_cast(opts.max_header_bytes)); stream->expires_after(std::chrono::seconds(opts.request_timeout_seconds)); auto& stream_ref = *stream; bhttp::async_read( @@ -91,6 +99,7 @@ struct BeastServerTransport::State : std::enable_shared_from_this { (void)stream->socket().shutdown(asio::ip::tcp::socket::shutdown_send, ignored); return; } + self->active.fetch_add(1); const bool keep_alive = parser->get().keep_alive() && !self->stopping; const HttpRequest request = ToSmithyRequest(parser->get()); // Handlers are synchronous for now (ADR-0003 keeps them exception-free); @@ -105,6 +114,9 @@ struct BeastServerTransport::State : std::enable_shared_from_this { wire_stream, wire_ref, [weak, stream, wire, keep_alive](beast::error_code write_ec, std::size_t) { auto self = weak.lock(); + if (self != nullptr) { + self->active.fetch_sub(1); + } if (self == nullptr || write_ec || !keep_alive) { beast::error_code ignored; (void)stream->socket().shutdown(asio::ip::tcp::socket::shutdown_send, ignored); @@ -180,17 +192,32 @@ void BeastServerTransport::Shutdown() noexcept { } state_->stopping = true; try { - // Stop the pool and join; once no io thread is running, it is safe to - // touch the acceptor directly (nothing may be posted into the io_context - // here — an abandoned handler would keep State alive and leak it). + // Stop accepting first: close the acceptor on its strand (the accept + // loop may be touching it on an io thread). The posted closure captures + // State weakly so an abandoned handler cannot keep State alive. + asio::post(state_->acceptor.get_executor(), [weak = std::weak_ptr(state_)] { + auto self = weak.lock(); + if (self != nullptr) { + boost::system::error_code ignored; + (void)self->acceptor.close(ignored); + } + }); + // Drain: requests already read get up to drain_timeout_seconds to finish + // writing; keep-alive is off (stopping), so served sessions then close. + const auto deadline = std::chrono::steady_clock::now() + + std::chrono::seconds(std::max(state_->opts.drain_timeout_seconds, 0)); + while (state_->active.load() > 0 && std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + // Now stop the pool and join; once no io thread is running, nothing may + // be posted into the io_context (an abandoned handler owning State would + // form a reference cycle and leak everything). state_->io.stop(); for (std::thread& thread : threads_) { if (thread.joinable()) { thread.join(); } } - boost::system::error_code ignored; - (void)state_->acceptor.close(ignored); } catch (...) { // Teardown must not propagate out of a destructor. } diff --git a/runtime/src/server/middleware.cc b/runtime/src/server/middleware.cc new file mode 100644 index 00000000..85b98368 --- /dev/null +++ b/runtime/src/server/middleware.cc @@ -0,0 +1,36 @@ +#include "smithy/server/middleware.h" + +#include +#include + +namespace smithy::server { + +http::RequestHandler Chain(std::vector middleware, http::RequestHandler handler) { + // Wrap inside-out so the first middleware ends up outermost. + for (const Middleware& wrap : middleware | std::views::reverse) { + handler = wrap(std::move(handler)); + } + return handler; +} + +Middleware Observe(std::function callback, + std::function now) { + if (now == nullptr) { + now = [] { return std::chrono::steady_clock::now(); }; + } + return [callback = std::move(callback), now = std::move(now)](http::RequestHandler next) { + return [callback, now, next = std::move(next)](const http::HttpRequest& request) { + const auto start = now(); + http::HttpResponse response = next(request); + RequestObservation observation; + observation.method = request.method; + observation.target = request.target; + observation.status = response.status; + observation.duration = std::chrono::duration_cast(now() - start); + callback(observation); + return response; + }; + }; +} + +} // namespace smithy::server diff --git a/runtime/tests/client/retry_test.cc b/runtime/tests/client/retry_test.cc index b2a77d89..847c7a76 100644 --- a/runtime/tests/client/retry_test.cc +++ b/runtime/tests/client/retry_test.cc @@ -3,6 +3,9 @@ #include #include +#include +#include +#include #include #include "smithy/core/error.h" @@ -97,6 +100,81 @@ TEST(SendWithRetriesTest, GivesUpAfterMaxAttempts) { EXPECT_EQ(transport.calls, 3); } +// Interceptor recording hook calls and stamping a header per attempt. +class RecordingInterceptor final : public Interceptor { + public: + void ModifyBeforeTransmit(http::HttpRequest& request, int attempt) override { + request.headers.Set("x-attempt", std::to_string(attempt)); + modify_attempts.push_back(attempt); + } + void ReadAfterTransmit(const http::HttpRequest& request, + const Outcome& outcome, int attempt) override { + seen_headers.push_back(request.headers.Get("x-attempt").value_or("")); + statuses.push_back(outcome.ok() ? outcome->status : -1); + (void)attempt; + } + + std::vector modify_attempts; + std::vector seen_headers; + std::vector statuses; +}; + +TEST(SendWithRetriesTest, InterceptorsRunAroundEveryAttempt) { + ScriptedTransport transport; + transport.script = {http::HttpResponse{503, {}, "busy"}, Error::Transport("refused"), + http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + auto interceptor = std::make_shared(); + const auto outcome = SendWithRetries(transport, {}, InstantPolicy(&slept), {interceptor}); + + ASSERT_TRUE(outcome.ok()); + EXPECT_EQ(interceptor->modify_attempts, (std::vector{1, 2, 3})); + // Each attempt mutates a fresh copy: the stamp never carries over. + EXPECT_EQ(interceptor->seen_headers, (std::vector{"1", "2", "3"})); + EXPECT_EQ(interceptor->statuses, (std::vector{503, -1, 200})); +} + +TEST(SendWithRetriesTest, InterceptorMutationsDoNotLeakIntoCallersRequest) { + ScriptedTransport transport; + transport.script = {http::HttpResponse{200, {}, "ok"}}; + std::vector slept; + http::HttpRequest request; + const auto outcome = SendWithRetries(transport, request, InstantPolicy(&slept), + {std::make_shared()}); + ASSERT_TRUE(outcome.ok()); + EXPECT_FALSE(request.headers.Get("x-attempt").has_value()); +} + +TEST(SendWithRetriesTest, InterceptorsRunInRegistrationOrder) { + std::vector slept; + + class AppendingInterceptor final : public Interceptor { + public: + explicit AppendingInterceptor(std::string tag) : tag_(std::move(tag)) {} + void ModifyBeforeTransmit(http::HttpRequest& request, int) override { + request.headers.Set("x-tags", request.headers.Get("x-tags").value_or("") + tag_); + } + + private: + std::string tag_; + }; + + class CapturingTransport final : public http::HttpClient { + public: + Outcome Send(const http::HttpRequest& request) override { + tags = request.headers.Get("x-tags").value_or(""); + return http::HttpResponse{200, {}, "ok"}; + } + std::string tags; + }; + + CapturingTransport capturing; + (void)SendWithRetries( + capturing, {}, InstantPolicy(&slept), + {std::make_shared("a"), std::make_shared("b")}); + EXPECT_EQ(capturing.tags, "ab"); +} + TEST(SendWithRetriesTest, MaxAttemptsOneDisablesRetries) { ScriptedTransport transport; transport.script = {http::HttpResponse{503, {}, "busy"}}; diff --git a/runtime/tests/http/beast_transport_test.cc b/runtime/tests/http/beast_transport_test.cc index 9523308f..184d1e90 100644 --- a/runtime/tests/http/beast_transport_test.cc +++ b/runtime/tests/http/beast_transport_test.cc @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -123,6 +124,53 @@ TEST(BeastTransportTest, RejectsOversizedBodies) { server.Stop(); } +TEST(BeastTransportTest, RejectsOversizedHeaders) { + BeastServerTransport server(BeastServerTransport::Options{.max_header_bytes = 1024}); + ASSERT_TRUE(server.Start([](const HttpRequest&) { return HttpResponse{200, {}, ""}; }).ok()); + SocketHttpClient client("127.0.0.1", server.port()); + HttpRequest request; + request.method = "GET"; + request.target = "/"; + request.headers.Set("x-huge", std::string(8 * 1024, 'h')); + const auto response = client.Send(request); + // Beast closes the connection on a header-limit violation; either a + // transport error or an HTTP error status is acceptable, but never a 200. + if (response.ok()) { + EXPECT_GE(response->status, 400); + } + server.Stop(); +} + +TEST(BeastTransportTest, StopDrainsInFlightRequests) { + std::atomic handler_entered{false}; + BeastServerTransport server(BeastServerTransport::Options{.drain_timeout_seconds = 5}); + ASSERT_TRUE(server + .Start([&](const HttpRequest&) { + handler_entered = true; + std::this_thread::sleep_for(std::chrono::milliseconds(300)); + return HttpResponse{200, {}, "drained"}; + }) + .ok()); + + const int port = server.port(); + Outcome response = HttpResponse{}; + std::thread caller([&] { + SocketHttpClient client("127.0.0.1", port); + response = client.Send(HttpRequest{"GET", "/", {}, ""}); + }); + while (!handler_entered) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + + // Stop while the handler is mid-request: the response must still be + // written in full before the pool is torn down. + server.Stop(); + caller.join(); + ASSERT_TRUE(response.ok()) << response.error().message(); + EXPECT_EQ(response->status, 200); + EXPECT_EQ(response->body, "drained"); +} + TEST(BeastTransportTest, StartupErrorsAreReported) { BeastServerTransport bad(BeastServerTransport::Options{.address = "not-an-address"}); EXPECT_FALSE(bad.Start([](const HttpRequest&) { return HttpResponse{}; }).ok()); diff --git a/runtime/tests/server/middleware_test.cc b/runtime/tests/server/middleware_test.cc new file mode 100644 index 00000000..208cecde --- /dev/null +++ b/runtime/tests/server/middleware_test.cc @@ -0,0 +1,108 @@ +#include "smithy/server/middleware.h" + +#include + +#include +#include +#include + +namespace smithy::server { +namespace { + +using std::chrono::milliseconds; + +http::HttpResponse Ok(const std::string& body) { + http::HttpResponse response; + response.status = 200; + response.body = body; + return response; +} + +// Middleware appending a tag on the way in (request seen) and out (response). +Middleware Tag(std::vector* log, const std::string& name) { + return [log, name](http::RequestHandler next) { + return [log, name, next](const http::HttpRequest& request) { + log->push_back(name + ":in"); + http::HttpResponse response = next(request); + log->push_back(name + ":out"); + return response; + }; + }; +} + +TEST(ChainTest, FirstMiddlewareIsOutermost) { + std::vector log; + auto handler = Chain({Tag(&log, "a"), Tag(&log, "b")}, [&](const http::HttpRequest&) { + log.push_back("handler"); + return Ok("done"); + }); + + const auto response = handler({}); + EXPECT_EQ(response.body, "done"); + EXPECT_EQ(log, (std::vector{"a:in", "b:in", "handler", "b:out", "a:out"})); +} + +TEST(ChainTest, EmptyChainIsTheHandler) { + auto handler = Chain({}, [](const http::HttpRequest&) { return Ok("plain"); }); + EXPECT_EQ(handler({}).body, "plain"); +} + +TEST(ChainTest, MiddlewareCanShortCircuit) { + bool reached = false; + auto reject = [](http::RequestHandler) { + return [](const http::HttpRequest&) { + http::HttpResponse response; + response.status = 401; + return response; + }; + }; + auto handler = Chain({reject}, [&](const http::HttpRequest&) { + reached = true; + return Ok("never"); + }); + + EXPECT_EQ(handler({}).status, 401); + EXPECT_FALSE(reached); +} + +TEST(ObserveTest, ReportsMethodTargetStatusAndDuration) { + std::vector observations; + auto clock_time = std::chrono::steady_clock::time_point{}; + auto now = [&clock_time] { + auto current = clock_time; + clock_time += milliseconds(7); // each call advances: duration = 7ms + return current; + }; + + auto handler = + Chain({Observe([&](const RequestObservation& o) { observations.push_back(o); }, now)}, + [](const http::HttpRequest&) { + http::HttpResponse response; + response.status = 404; + return response; + }); + + http::HttpRequest request; + request.method = "GET"; + request.target = "/cities/1"; + (void)handler(request); + + ASSERT_EQ(observations.size(), 1u); + EXPECT_EQ(observations[0].method, "GET"); + EXPECT_EQ(observations[0].target, "/cities/1"); + EXPECT_EQ(observations[0].status, 404); + EXPECT_EQ(observations[0].duration, milliseconds(7)); +} + +TEST(ObserveTest, CountsEveryRequest) { + int count = 0; + auto handler = Chain({Observe([&](const RequestObservation&) { ++count; })}, + [](const http::HttpRequest&) { return Ok("ok"); }); + (void)handler({}); + (void)handler({}); + (void)handler({}); + EXPECT_EQ(count, 3); +} + +} // namespace +} // namespace smithy::server