Add IHttpResponseTrailersFeature and extensions #1043
Conversation
| public static void AppendTrailer(this HttpResponse response, string trailerName, StringValues trailerValues) | ||
| { | ||
| var feature = response.HttpContext.Features.Get<IHttpResponseTrailersFeature>(); | ||
| feature?.Trailers?.Append(trailerName, trailerValues); |
There was a problem hiding this comment.
Checking for null when you're already checking for the feature existence seems like it might be a little too defensive. Technically response.Headers could be set to null too, but there's no check in DeclareTrailer.
Is the idea that the server might set the feature but leave the IHeaderDictionary null? Or are you just defending against some middleware setting Trailers to null?
There was a problem hiding this comment.
Just paranoia. We make a lot of assumptions about the request and response features because nothing would work without them, but for these very optional features I try not to assume anything.
There was a problem hiding this comment.
Is it weird that this can noop? Should it throw if trailers aren't supported?
There was a problem hiding this comment.
If it threw you'd need some other way to check for support before calling it. Checking for the presence of the feature is the simplest, but negates using an extension like this. I could add another extension here SupportsTrailers to check for the feature.
We also discussed some Http/1.1 scenarios where the feature may be present but the headers may be discarded with a log message (e.g. content-length)
There was a problem hiding this comment.
Err hmm, I'm not sure if I like that, it's very similar to adding headers after the response body has started. Today we throw. I like that model.
|
Updated to throw if Trailers can't be added. |
src/Microsoft.AspNetCore.Http.Abstractions/Extensions/ResponseTrailerExtensions.cs
Show resolved
Hide resolved
| var feature = response.HttpContext.Features.Get<IHttpResponseTrailersFeature>(); | ||
| if (feature?.Trailers == null || feature.Trailers.IsReadOnly) | ||
| { | ||
| throw new InvalidOperationException("Trailers are not supported for this response."); |
There was a problem hiding this comment.
Nit: Could potentially consider adding a test for this.
Tratcher
force-pushed
the
feature/tratcher/trailers
branch
from
4007f24 to
89b0430
Compare
Part of https://github.com/aspnet/KestrelHttpServer/issues/622