Add response trailer feature

[khellang]

Opening this up for discussion, starting out with response trailers. We can attack request trailers later. This addresses the HttpAbstractions part of https://github.com/aspnet/KestrelHttpServer/issues/622...

My current thinking is that you add the trailing header names using the regular Headers dictionary:

response.Headers.Append("Trailer", "Server-Timing");

This would automatically chunk the response, if possible and allowed. You can, of course, also add headers with the same name:

response.Headers.Append("Server-Timing", "db;dur=53");
response.Headers.Append("Server-Timing", "app;dur=47.2");

These are, as expected, flushed together with the rest of the headers. You can then explicitly add trailers:

trailersFeature.OnStarting(() =>
{
    // This would be the very last thing to happen before it's flushe to the client.
    trailersFeature.Trailers.Append("Server-Timing", "total;dur=123.4");
});

The entire response would look something like this:

HTTP/1.1 200 OK
Server-Timing: db;dur=53, app;dur=47.2
Trailer: Server-Timing
(... snip response body ...)
Server-Timing: total;dur=123.4

I'm thinking it's best to keep the feature by itself, as it doesn't make sense to expose such a niche feature on the HttpResponse itself.

There are a couple of open questions, mostly related to implementation behavior:

1. What happens if expected trailers are missing from the Trailers dictionary (after OnStarting has been called)?
2. What happens if other (unexpected) trailers are added to the Trailers dictionary?
3. What kind of validation needs to be in place for Trailer entries? The RFC is pretty strict on which headers are allowed as trailers:

   A sender MUST NOT generate a trailer that contains a field necessary
   for message framing (e.g., Transfer-Encoding and Content-Length),
   routing (e.g., Host), request modifiers (e.g., controls and
   conditionals in Section 5 of [RFC7231]), authentication (e.g., see
   [RFC7235] and [RFC6265]), response control data (e.g., see Section
   7.1 of [RFC7231]), or determining how to process the payload (e.g.,
   Content-Encoding, Content-Type, Content-Range, and Trailer).

// @Tratcher @benaadams @Drawaes @NickCraver @serialseb @halter73

[halter73]

Nit: Maybe call this OnWritingResponseTrailers.

I get that my suggested name is a little redundant with the name of the interface, but I think the current naming is too similar to IHttpResponseFeature.OnStarting and could cause confusion.

[serialseb]

There's already an .OnSendingHeaders, maybe an .OnSendingTrailerHeaders would work?

[khellang]

There's an .OnSendingHeaders? I think that's the OWIN key. It's called OnStarting in the ASP.NET Core abstractions. That's why I chose this name, for consistency. I'm fine with whatever.

HttpAbstractions/src/Microsoft.AspNetCore.Http.Features/IHttpResponseFeature.cs

Lines 42 to 49 in b85ed9d

	/// <summary>
	/// Registers a callback to be invoked just before the response starts. This is the
	/// last chance to modify the <see cref="Headers"/>, <see cref="StatusCode"/>, or
	/// <see cref="ReasonPhrase"/>.
	/// </summary>
	/// <param name="callback">The callback to invoke when starting the response.</param>
	/// <param name="state">The state to pass into the callback.</param>
	void OnStarting(Func<object, Task> callback, object state);

[serialseb]

Ah. Yes I see what you mean. Shame it wasn't called something more like the owin feature then. Consistency would probably win here.

[halter73]

WasWritten?

[khellang]

That would indicate that it's set after the trailers has been written? This would be set just before the server starts writing.

[halter73]

I'm not too convinced WasWritten is a better name either.

I proposed WasWritten since the Trailers dictionary is closed for modification and the server has committed to writing what's in the dictionary as trailers at the point this property is set. From the perspective of the app developer, I don't see how committing to writing the trailers is observably different from actually writing the trailers especially given that there's write-behind response buffer.

HasStarted isn't bad though. I was thinking HasStartedWriting to make it less ambiguous, but I suggested WasWritten since that's shorter.

If we decided to change OnStarting to OnStartingTrailers or something similar, we might want to change this to HasStartedTrailers instead.

[khellang]

Agan, I chose HasStarted because it's consistent with IHttpResponseFeature.HasStarted. If we can find another name, or get rid of the property entirely, that's cool with me.

[Tratcher]

IsReadOnly? IHeaderDictionary already has that property, we can probably remove this one.

[serialseb]

Why would the trailer dictionary not be mutable? I think I'm missing something in the IsReadOnly comment

Ok, got it. On the trailer header dictionary. Makes sense.

[halter73]

I'm not too familiar with the Trailer spec, so I don't know what should be done as far as validation goes. I do like have some sort of callback that gets invoked before writing trailers though.

[serialseb]

What happens if expected trailers are missing from the Trailers dictionary (after OnStarting has been called)?

Of interest is the current conversations in http bis and fetch. For example httpwg/http-core#16

From this, I'd think that if you say "i'll send trailers", the fact that you do in the end or not is not problematic for the client, so I'd assume it's ok to not send them. Plus, if it wasn't, you're way way way down in the response sending, so there's little you can do about it.

What happens if other (unexpected) trailers are added to the Trailers dictionary?

From a dev perspective, magic = bad, so i'd throw immediately if you try and write a trailer you didn't say you would write before the non-trailered headers are sent. Silently erroring out doesn't help anyone in this context.

What kind of validation needs to be in place for Trailer entries? The RFC is pretty strict on which headers are allowed as trailers:

Headers that are forbidden to be sent in trailers from the 1.1 spec could throw, as there is no way anyone could find any use whatsoever for breaking the spec. Knowning that, it means that you'll ever only be able to validate those headers you decided on at that point, as adding more forbidden headers in the future would break compat.

So i'd thnk throw on 1.1 forbidden headers, and if that list gets extended in the future, then add an opt out for people upgrading. It's easier to stop an exclusion than to start an exclusion

[serialseb]

Of interest, in go:
https://go-review.googlesource.com/c/go/+/2157
And how they validate in h11 vs h2 golang/go#23908

in nodejs: https://nodejs.org/api/http.html#http_response_addtrailers_headers

May be interesting to have a look at other platforms

[khellang]

Yeah, looks like this is a good list;

https://github.com/golang/net/blob/cbe0f9307d0156177f9dd5dc85da1a31abc5f2fb/http2/server.go#L2841-L2874

[khellang]

Hmm. What do you think about just a single method like

void DeclareTrailer(string key, Func<object, Task<StringValues>> callback, object state)

You'd have to call the method to declare all trailers before IHttpResponseFeature.OnStarting (when the headers are flushed) is called. This would automatically give you the information to produce a correct Trailer header, plus a callback to produce a value for each trailer.

It also answers the other questions, as there would be no way to add more (or less) trailers than those declared in the header, plus you can validate them all up-front, before the response has started.

[serialseb]

That looks good. I'm gonna have a think and see if there are scenarios this wouldn't allow for.

[Tratcher]

@khellang not a fan of DeclareTrailer, that looks like a higher layer wrapper over the current minimal mechanics. Consider if you have one component declaring the trailers and another providing them.

[Tratcher]

IsReadOnly? IHeaderDictionary already has that property, we can probably remove this one.

[Tratcher]

Where do you expect to need this implementation? I expect the servers to provide their own.

[khellang]

Where do you expect to need this implementation?

I don't. I just saw the other features had "dummy" implementations, so just followed along.

[khellang]

that looks like a higher layer wrapper over the current minimal mechanics.

Maybe. The problem is just that I think these mechanics are too minimal. Hence the questions in the initial post. The DeclareTrailer method answers them all (I think).

Consider if you have one component declaring the trailers and another providing them.

Yeah, that's problematic for many reasons. How do you coordinate between those components? Do you have an example in mind?

[Tratcher]

The feature interfaces are supposed to be as low level and as flexible as possible. It's only at the HttpContext layer that we try to provide more explicit usage patterns.

A basic example would be that I know all responses will included some set of trailers. It's quite easy and most efficient to declare a flat list up front. However, the code responsible for generating each those values is distributed throughout the response processing. Consider one that gives you database time, one for serialization time, etc.. With DeclareTrailer you'd have to stash that information as it's generated and then retrieve it later when the callback is invoked. I'd much rather be able to add it to the response when it's generated.

[Tratcher]

Separate question: is symmetry important? Are there scenarios we're interested in that require both request and response trailers? Or are response trailers the majority case?

I ask because Http.Sys can't support request trailers, but it can support response trailers. Not sure if you can do response trailers on IIS. Kestrel can do both.

[serialseb]

With DeclareTrailer you'd have to stash that information as it's generated and then retrieve it later when the callback is invoked. I'd much rather be able to add it to the response when it's generated.

Ok, I delved a bit more into this. For example, in OpenRasta we have a single tap pipeline that gets auto wired-up at various stages in http processing, one being response coding, which a component can declare to want to execute before of. We don't currently have a separate part of the pipeline for ResponseSending, they're assumed to be one and the same. Trailers would allow me to add an extra KnownStages.IResponseTrailerSending and let the system wire up components at the right time. Without the notification, i have nothing to hang the pipeline off of. So the DeclareTrailer would make it slightly more complicated.

I attach some code that would be what I expect to write:

public class ResponseWriteTimings : IPipelienContributor
{
  StopWatch timer;
  public void Initialize(IPipeline builder) {
    builder.Notify(RegisterTrailer).Before<KnownStage.IResponseCoding();
    builder.NotifyAsync(WriteTrailer).Before<KnownStages.IResponseTrailerSending();
  }
  void RegisterTrailer(ICommunicationContext context) {
    context.Response.Trailer += "Server-Timing";
   // initialise StopWatch
  }
  void WriteTrailer(ICommunicationContext context) {
    context.Response.Headers.ServerTiming += new Timing(stopWatch.ElapsedMilliseconds);
 }
}

Any model that wouldn't break something along those lines would allow us to implement that feature, when the server supports it.

Request trailers would work the same way, so I would need a rather symetric appraoch for the request pipeline.

On a side note, as we don't use the asp.net object model for http headers, i'd rather we had as low level as possible access to headers, so any object model deviating from that would make me less happy.

[khellang]

@serialseb I'm curious how you're planning to tackle the questions mentioned in the top post.

Looking at the example you posted, there's still some unanswered questions:

1. What happens if you declare a trailer, but forget to actually append it in the WriteTrailer callback?
2. What happens if you've forgotten to declare a trailer, but still try to append it in WriteTrailer? At that point, it's already too late to change headers, as they've already been flushed to the client.
3. I see you're using context.Response.Headers to append trailers. How are you planning on validating legal trailers. The RFC is pretty strict on which headers can be sent as trailers.

Are you implementing OpenRasta on top of the ASP.NET Core abstractions? I'm assuming you have to re-implement a lot of these checks to get a consistent experience across different hosts?

[khellang]

not a fan of DeclareTrailer, that looks like a higher layer wrapper over the current minimal mechanics. Consider if you have one component declaring the trailers and another providing them.

@Tratcher How do you propose we answer the questions above when you can have some components that declare and some components that provide trailers? How do we make sure they're consistent? Do we need to?

[Tratcher]

The server's job is to make sure the client gets an intelligible response. Most of these mistakes wouldn't corrupt the response, the client would still be able to read it but it may end up with more or less information than it was expecting. Do we have any clients that actually surface trailers to do some interop testing with?

[khellang]

Chrome 65 supports Server-Timing. I'll see if I can test some of the scenarios.

[serialseb]

What happens if you declare a trailer, but forget to actually append it in the WriteTrailer callback?

That shouldn't be a problem, a trailer is just a lose promise, from what i understand of the spec. I may send a Server-Timing but I may not

What happens if you've forgotten to declare a trailer, but still try to append it in WriteTrailer? At that point, it's already too late to change headers, as they've already been flushed to the client.

Well that's a server error but the response, I'd flag / warn about it but I can't really do much about it.

I see you're using context.Response.Headers to append trailers. How are you planning on validating legal trailers. The RFC is pretty strict on which headers can be sent as trailers.

Well, it's pretty strict as to which headers cannot be sent :) So i'd probably make sure i implement the spec as correctly as possible.

That said it's entirely possible that i'd build an api on top of the low level pipeline one above, but at that point I can implement the delegate on top of it, the other way around is not possible.

[khellang]

This was superseded by #1043