feat: move close-body, logger, and service-metadata work out of the middleware stack by Tongs2000 · Pull Request #683 · aws/smithy-go

Tongs2000 · 2026-07-06T22:52:21Z

Issue #, if available:

N/A

Description of changes:

Reduce the number of per-request middlewares on every operation's stack by moving work that does not need to be a standalone middleware into codegen: into the generated operation deserializer or invokeOperation. Each removed middleware saves at least 2 allocations per request (the struct plus its handler wrapper) on a known hot path.

Three independent changes, all in codegen plus shared runtime helpers:

1. Close response body in the operation deserializer (Deserialize -1)

Adds CloseResponseBody(ctx, resp, isStreaming, opErr), a shared function that closes the response body. It leaves the body open only for a successful response whose payload is a caller-owned stream (isStreaming true with a nil opErr); on error, or for a non-streaming response, it always closes the body (an error response body is diagnostic, not a caller-owned stream).
Codegen emits a deferred CloseResponseBody call at the top of each generated operation deserializer, across the HttpRpc, HttpBinding, and protocol/serde2 deserializer generators. The call is deferred in a closure (defer func() { CloseResponseBody(..., err) }()) so it observes the final operation error.
Streaming detection differs by path. For the legacy HttpRpc / HttpBinding generators, isStreaming is decided at codegen time via ProtocolUtils.isCallerOwnedResponseStream (event stream, or a @streaming payload). For serde2, the per-service middleware cannot see the operation at codegen time, so it is decided at runtime: m.operationSchema.IsOutputEventStream() for event streams, or the smithy.StreamingOutput interface for a @streaming payload output.
The standalone close-response-body middleware is no longer registered; the now-unused HttpProtocolUtils.getCloseResponseClientPlugins helper is removed.
AddCloseResponseBodyMiddleware and AddErrorCloseResponseBodyMiddleware are kept and marked // Deprecated (they now delegate to CloseResponseBody) so external callers are not broken.

2. Operation context resolver codegen hook

Adds a RuntimeClientPlugin extension point, operationContextResolvers: a set of symbols pointing to functions with the signature func(context.Context, Options, string) context.Context. invokeOperation emits ctx = resolver(ctx, options, opID) for each, after options are finalized and before the middleware stack is built.
This lets a plugin set request-scoped context values directly in invokeOperation instead of via a per-request Initialize-step middleware, for values that live in a keyspace the smithy-go base template cannot reference.

3. Set the logger via the context resolver hook (Initialize -1)

The setLogger middleware only stashed options.Logger into the request context at the start of Initialize. ClientLogger now generates a setLoggerContext resolver (return middleware.SetLogger(ctx, options.Logger)) and registers it through the operation context resolver hook above, so the logger is set in invokeOperation instead of by a standalone middleware.
ClientLogger no longer registers a middleware; it still contributes the Logger config field and the default-logger resolver. The public AddSetLoggerMiddleware function is unchanged.

lucix-aws · 2026-07-16T20:56:02Z


    @Override
    public List<RuntimeClientPlugin> getClientPlugins() {
+        // The logger is set on the context directly in invokeOperation


shouldn't this now have an OperationContextResolver instead? if someone overrides this integration the generated code that references options.Logger elsewhere will be broken

Good catch, agreed. Done. ClientLogger now generates a setLoggerContext resolver and registers it via addOperationContextResolver, and I removed the hardcoded SetLogger line from the invokeOperation template. So overriding the integration removes the logger setup too, no dangling options.Logger reference.

lucix-aws · 2026-07-16T21:08:50Z

    public abstract Writable generateDeserialize();

    private Writable generateHandleDeserialize() {
+        // Close body on success unless the output is a caller-owned event stream.


dont both of these serialize/deserialize middleware changes also need replicated in the schema-serde path? this is the legacy serde path

Good call. This is the legacy path. serde2 relied on the same standalone close/content-length middlewares (registered for all services), which this change removes, so I've replicated the inline behavior in the serde2 path:

Serde2DeserializeResponseMiddleware: defer CloseResponseBody after deserialize, skipping caller-owned streams (IsOutputEventStream() or the smithy.StreamingOutput interface on the output).

Serde2SerializeRequestMiddleware: compute content length after serialize, skipping event-stream inputs.

Streaming is detected at runtime here since the serde2 middlewares are per-service rather than per-operation, but the behavior matches the legacy paths.

lucix-aws · 2026-07-16T21:14:04Z

 	"github.com/aws/smithy-go/middleware"
 )

+// DrainAndCloseResponseBody drains and closes the HTTP response body. It always


for something we're adding to the runtime like this we should really just explicitly parameterize it rather than have behavior flags that are documented as "pass x when y".

e.g. DrainAndClose(ctx, resp, err, isStreaming)

Agreed, done. Switched to an explicit isStreaming param. It actually simplified further than your example: since we're dropping the success-path drain (the body's already consumed during deserialization), the success and error paths do the same thing, so the err param isn't needed either. It's now CloseResponseBody(ctx, resp, isStreaming).

lucix-aws · 2026-07-16T21:17:33Z

+	}
+
+	if opErr != nil {
+		// Consume the full body to prevent TCP connection resets on some platforms.


I know this is existing but I'm wondering if we really care to do this at all. In the success case we know we read the whole body to deserialize it so it's pointless. If at this point we did not read the whole response body it's either because

it's a streaming payload and it's the caller's problem

we had an actual error reading the response body, which means any further attempt to read it would probably also error, and also if this claim is true about connection reuse (idk if it is tbh) we probably want to throw that connection away anyway

Agreed. Removed the drain entirely. Success path: the body's already fully read during deserialization, so there's nothing to drain. Error path: as you said, a failed read won't get better by re-reading, and we'd want to discard that connection anyway. So CloseResponseBody now just closes (unless it's a caller-owned stream) and never drains.

lucix-aws · 2026-07-16T21:19:44Z


+                // Close the body on every exit path in place of the standalone close middleware.
+                resp, _ := out.RawResponse.($response:P)
+                defer $drainClose:T(ctx, resp, $closeOnSuccess:L, err)


arguments passed to defer are evaluated/captured when defer is called, so if err gets set to something else later you're dropping its value

Good catch. Rather than wrap it in a closure to capture the final err, the reworked function doesn't take err at all anymore (dropping the drain made the success/error paths identical, so there's nothing to branch on). The generated call is now defer CloseResponseBody(ctx, resp, isStreaming) with no err, so the early-capture problem goes away entirely.

Following up on my previous reply: I walked this back. Dropping err was wrong for streaming outputs.

The concern is the error path for a caller-owned stream. If an operation whose output is a streaming payload (or event stream) fails during deserialization, isStreaming is true, so a no-err CloseResponseBody(ctx, resp, isStreaming) would skip the close and leak the body, since there is no successful
stream for the caller to consume and close. The standalone error-close middleware closed it in that case, so dropping err would have been a behavior change (the kitchensink test caught exactly this: streaming output + error path, body not closed).

So the function keeps err: CloseResponseBody(ctx, resp, isStreaming, opErr). It leaves the body open only when isStreaming && opErr == nil (a successful caller-owned stream); on error, or for a non-streaming response, it always closes. This preserves the original behavior on every path.

To your original point about defer capturing err too early: the generated call is a closure, defer func() { CloseResponseBody(ctx, resp, isStreaming, err) }(), so it reads the final err at return time rather than capturing it at the defer line. err is a named return value in the generated
deserializer, so the closure sees whatever the deserialize logic last set.

lucix-aws · 2026-07-16T21:20:39Z


    private Writable generateHandleDeserialize() {
+        // Close body on success unless the output is a caller-owned event stream.
+        boolean closeOnSuccess = EventStreamIndex.of(ctx.getModel()).getOutputInfo(operation).isEmpty();


pretty sure this should also be the case for @streaming blob? e.g. GetObject

Good catch. And while doing it I noticed the three deserialize paths had drifted: HttpBinding checked both event streams and @streaming payloads, but HttpRpc and this serde path only checked event streams. I pulled the check into a shared ProtocolUtils.isCallerOwnedResponseStream(model, operation) (event stream OR @streaming payload) and pointed all three paths at it, so they can't drift again.

lucix-aws · 2026-07-16T21:26:03Z

We are probably going to want some new kitchen sink tests for these things over in the SDK (internal/kitchensinktest, it's just a bogus generated client with an operation or two that you can write tests against). especially with the response body close stuff, it would be good to have tests actually go into main right now (before this is merged) that verify the behavior so we can check it didn't change overall here.

wty-Bryant · 2026-07-21T18:28:09Z

-            });
+            writer.openBlock("func $L(ctx $T, options Options, operation string) $T {", "}",
+                    SET_LOGGER_CONTEXT_RESOLVER, contextSymbol, contextSymbol, () -> {
+                        writer.write("_ = operation");


Is there any other resolver func implementing this func(ctx, options, string) required by operationContextResolvers? Asked since the opId here is just dropped
My understanding is this opId is kept for forward-compatibility once any other operation ctx resolvers are added

Yes. The operation argument is consumed by the paired aws-sdk-go-v2 PR (#3480), which contributes a resolveServiceMetadata resolver of this same signature:

func resolveServiceMetadata(ctx context.Context, options Options, operation string) context.Context { ctx = awsmiddleware.SetServiceID(ctx, ServiceID) if options.Region != "" { ctx = awsmiddleware.SetRegion(ctx, options.Region) } ctx = awsmiddleware.SetOperationName(ctx, operation) // <- uses opID if options.EndpointResolver != nil { ctx = awsmiddleware.SetRequiresLegacyEndpoints(ctx, true) } return ctx }

This replaces the old per-operation RegisterServiceMetadata Initialize middleware. That middleware stamped the operation name into the request context, and downstream code reads it back via GetOperationName(ctx):

aws/retry/attempt_metrics.go sets rpc.method on the metrics from it

aws/retry/middleware.go logs service, operation on retries

feature/s3/manager branches on GetOperationName(ctx) == "CreateMultipartUpload"

So the opID has to reach the context somehow.

It now needs to be passed in because the old middleware was generated per operation, so each instance carried its own name hard-coded in the struct (OperationName: "ListQueues") and just read s.OperationName at runtime, no argument needed. Folding it into invokeOperation collapses that into a single shared
resolveServiceMetadata function used by every operation, which therefore can't carry any one operation's name. The only way it still knows which operation is running is the opID that invokeOperation already has, passed in via the hook: ctx = resolveServiceMetadata(ctx, options, opID).

setLoggerContext is the one in-tree (smithy-go) resolver, and the logger genuinely doesn't depend on the operation, so it drops the arg (_ = operation). The third parameter is part of the shared contract for resolvers that do need it, resolveServiceMetadata being the motivating case.

…dalone middleware Add DrainAndCloseResponseBody and emit a deferred call to it from the generated operation deserializer (HttpRpc, HttpBinding, and protocol/serde2 rpc2 paths), removing the need for a standalone close-response-body middleware on the stack. closeOnSuccess is decided at codegen time: false for caller-owned streams (streaming payloads and event-stream outputs), true otherwise. The existing Add*CloseResponseBodyMiddleware functions are retained for backwards compatibility.

The setLogger middleware only stashed options.Logger into the request context at the start of the Initialize step. That value is static for the whole operation, so it can be set directly in invokeOperation alongside the existing WithServiceID / WithOperationName calls, removing a per-request middleware from every operation's stack. ClientLogger no longer registers a middleware; it still contributes the Logger config field and the default-logger resolver. The public AddSetLoggerMiddleware function is unchanged (still available for external callers).

Adds a RuntimeClientPlugin extension point, operationContextResolvers: a set of symbols pointing to functions with the signature func(context.Context, Options, string) context.Context. invokeOperation emits ctx = resolver(ctx, options, opID) for each, after options are finalized and before the middleware stack is built. This lets a plugin set request-scoped context values directly in invokeOperation instead of via a per-request Initialize-step middleware, for values (like the ones RegisterServiceMetadata sets) that live in a keyspace the smithy-go base template can't reference. No generated code changes until a plugin uses it.

…iddleware The ComputeContentLength middleware only set req.ContentLength from the serialized body length at the Build step. That value is available as soon as the body is serialized, so the generated operation serializer now computes it inline via a new exported smithyhttp.ComputeRequestContentLength(req), removing a per-request middleware from the stack. - transport/http: extract the compute logic into ComputeRequestContentLength; ComputeContentLength / AddComputeContentLengthMiddleware are kept but marked Deprecated (public API), now delegating to the shared function. - codegen: emit the call at the end of the operation serializer in all three paths (HttpRpc, HttpBinding, protocol/serde2). Skipped for event-stream inputs, matching the previous middleware registration condition. The call runs before the request leaves the serialize step, so downstream consumers that read req.ContentLength (SigV4/v4a signer, checksum) still see the computed value.

@streaming

- CloseResponseBody: drop the drain and the closeOnSuccess/err params, take isStreaming instead. The body is already consumed during deserialization, so draining is a no-op; success and error paths now just close (unless it is a caller-owned stream). This also removes the defer-captures-err-early bug, since the call no longer takes err. - Share the streaming detection via ProtocolUtils.isCallerOwnedResponseStream (event stream OR @streaming payload), so all three deserializer codegen paths (HttpRpc, HttpBinding, serde2) use one implementation instead of three hand-written copies that had drifted (HttpRpc/serde2 only checked event streams and missed streaming blobs like S3 GetObject).

Move the SetLogger call out of the invokeOperation base template and into an operation context resolver contributed by ClientLogger, matching how service metadata is set. ClientLogger now generates a setLoggerContext resolver and registers it via addOperationContextResolver, so overriding the integration removes the logger setup too instead of leaving a dangling options.Logger reference in the generated template.

@streaming

The schema-serde (serde2) middlewares relied on the standalone close-response-body and content-length middlewares, which this change removes. Replicate the inline behavior in the serde2 path to match the legacy paths: - Serde2DeserializeResponseMiddleware: defer CloseResponseBody after the response is deserialized, skipping caller-owned streams (event stream via OperationSchema.IsOutputEventStream, or a @streaming payload detected via the smithy.StreamingOutput interface on the output). - Serde2SerializeRequestMiddleware: compute the request content length once the body is serialized, skipping event-stream inputs. Streaming is detected at runtime here (the serde2 middlewares are per-service, not per-operation) rather than at codegen time as in the legacy paths, but the resulting behavior matches.

A streaming/event-stream response body is caller-owned only on success. On error the body is a diagnostic error payload, not a caller-owned stream, so it must still be closed — the previous change skipped it whenever the output was streaming, regressing the old ErrorCloseResponseBody behavior and leaking the connection on failed streaming requests. CloseResponseBody now takes the operation error and only leaves the body open for a successful streaming response (isStreaming && opErr == nil). The generated deserializers defer it in a closure so it observes the final err rather than capturing it early.

Content length must be computed after all body-mutating middleware have run. Request compression is a Serialize-step middleware that replaces the body with its gzip-compressed form; computing content length inline in the operation serializer runs before compression, so a compressed request would advertise the uncompressed length. The standalone ComputeContentLength middleware runs in the Build step (after Serialize), which is the stable anchor that guarantees it sees the final body. Reverts the inlined ComputeRequestContentLength across the HttpRpc, HttpBinding, rpc2, and serde2 serializer generators, and removes the now-unused ComputeRequestContentLength runtime function. The close-body, logger, and service-metadata changes are unaffected; only content length returns to a middleware.

Moving response-body close into the operation deserializer regressed event stream operations: the inner deserializer closed the body even for event streams, whose body is owned by the event stream deserializer. On the error path of a bidirectional stream the write side may still be active, so closing the HTTP/2 body there deadlocks (observed as a hang in bedrockruntime InvokeModelWithBidirectionalStream_ResponseError). Skip emitting the close in the operation deserializer for event stream operations across all protocol generators (HTTP binding, HTTP RPC, protocol, serde2). Streaming payloads (e.g. S3 GetObject) are unaffected. This restores the prior behavior where event stream operations did not register the close-response-body middlewares.

lucix-aws · 2026-08-06T16:35:59Z

 ) {
 	out, metadata, err := next.HandleDeserialize(ctx, input)
 	if err != nil {
-		if resp, ok := out.RawResponse.(*Response); ok && resp != nil && resp.Body != nil {


Is there any reason you're changing these deprecated implementations? can't they just stay as-is?

You're right, will revert to the original implementation. This was from an earlier iteration where I was consolidating both paths before deciding to deprecate. Since they're deprecated now it doesn't matter either way, but no reason to touch them.

lucix-aws · 2026-08-06T16:41:49Z


-            writer.write("response, ok := out.RawResponse.($P)", responseType);
-            writer.openBlock("if !ok {", "}", () -> {
+            writer.openBlock("if response == nil {", "}", () -> {


~~why change this?~~

i think it relates to the above

lucix-aws · 2026-08-06T16:44:19Z

+
+            // Close the response body after deserialization (a streaming payload is
+            // kept open on success). Event streams close their own body.
+            writer.write("response, _ := out.RawResponse.($P)", responseType);


you really need to keep the explicit type check here, I know that you have the nil guard in CloseResponseBody but you don't have to if you're doing the proper (response, ok -> check ok) like we do everywhere else

basically the pattern should always be, for deserialize

out,md,err = next.HandleDeserialize if err != nil { return out,md,err } resp, ok := out.whatever.(smithyhttp.Response) if !ok { return out, md, error about wrong transport } proceed

Acknowledged on all of these. Will update all three generators (HttpBinding, HttpRpc, serde2)

lucix-aws · 2026-08-06T16:45:17Z

+
+            // Close the response body after deserialization (a streaming payload is
+            // kept open on success). Event streams close their own body.
+            writer.write("response, _ := out.RawResponse.($P)", responseType);


same deal as above

lucix-aws · 2026-08-06T16:45:33Z

                out, metadata, err = next.HandleDeserialize(ctx, in)

+                // Close the response body once deserialization is done.
+                resp, _ := out.RawResponse.($response:P)


same as above

Address review feedback: the generated operation deserializer middlewares now follow the canonical shape used elsewhere -- check next's err first, then a single `resp, ok := out.RawResponse.(...)` assertion with an explicit `!ok` transport-error return, instead of a `_`-discarded assertion plus a nil guard. The deferred CloseResponseBody is registered after the `!ok` check so it still observes the final err on the deserialize (status-code) error path. Applied across the HTTP binding, HTTP RPC, protocol, and serde2 generators. Also revert the deprecated errorCloseResponseBodyMiddleware and closeResponseBody implementations to their original form (they keep their io.Copy drain rather than delegating to CloseResponseBody); they are deprecated and unused, so there is no reason to change their behavior.

lucix-aws · 2026-08-06T19:47:06Z

approve pending CI pass AND downstream SDK CI pass

Tongs2000 marked this pull request as ready for review July 7, 2026 21:02

Tongs2000 requested review from a team as code owners July 7, 2026 21:02

Tongs2000 force-pushed the feat-combine-middlewares branch 3 times, most recently from 8aae7ed to 8566e94 Compare July 14, 2026 16:11

Tongs2000 changed the title ~~feat: merge close-response-body middlewares into one~~ feat: close response body in the operation deserializer instead of a standalone middleware Jul 14, 2026

Tongs2000 changed the title ~~feat: close response body in the operation deserializer instead of a standalone middleware~~ feat: reduce per-operation middlewares by moving stack work into codegen Jul 16, 2026

Tongs2000 changed the title ~~feat: reduce per-operation middlewares by moving stack work into codegen~~ feat: move close-body, logger, service-metadata, and content-length work out of the middleware stack Jul 16, 2026

Tongs2000 force-pushed the feat-combine-middlewares branch from b7f59f3 to 1234a40 Compare July 16, 2026 18:02

lucix-aws reviewed Jul 16, 2026

View reviewed changes

wty-Bryant reviewed Jul 21, 2026

View reviewed changes

Tongs2000 force-pushed the feat-combine-middlewares branch from 1311512 to 491d181 Compare July 27, 2026 23:31

Tongs2000 mentioned this pull request Jul 28, 2026

feat: move close-body, logger, and service-metadata work out of the middleware stack aws/aws-sdk-go-v2#3480

Merged

Tongs2000 changed the title ~~feat: move close-body, logger, service-metadata, and content-length work out of the middleware stack~~ feat: move close-body, logger, and service-metadata work out of the middleware stack Jul 28, 2026

Tongs2000 force-pushed the feat-combine-middlewares branch from 491d181 to e968570 Compare July 30, 2026 19:19

Tongs2000 added 8 commits July 31, 2026 21:00

Tongs2000 added 2 commits July 31, 2026 21:00

Tongs2000 force-pushed the feat-combine-middlewares branch from e968570 to 52f5e60 Compare August 1, 2026 03:46

lucix-aws reviewed Aug 6, 2026

View reviewed changes

lucix-aws approved these changes Aug 6, 2026

View reviewed changes

trigger CI re-run

a8fb27e

Tongs2000 merged commit 1380d84 into main Aug 7, 2026
4 checks passed

lucix-aws deleted the feat-combine-middlewares branch August 7, 2026 18:17

Uh oh!

Conversation

Tongs2000 commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucix-aws Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tongs2000 Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tongs2000 Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucix-aws Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucix-aws commented Jul 16, 2026

Uh oh!

wty-Bryant Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucix-aws Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucix-aws commented Aug 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tongs2000 commented Jul 6, 2026 •

edited

Loading

lucix-aws Jul 16, 2026 •

edited

Loading

Tongs2000 Jul 17, 2026 •

edited

Loading

Tongs2000 Jul 17, 2026 •

edited

Loading

lucix-aws Jul 16, 2026 •

edited

Loading

wty-Bryant Jul 21, 2026 •

edited

Loading

lucix-aws Aug 6, 2026 •

edited

Loading