aspnet / HttpAbstractions

[Archived] HTTP abstractions such as HttpRequest, HttpResponse, and HttpContext, as well as common web utilities. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
382 stars 193 forks source link

Add response trailer feature #1000

Closed khellang closed 6 years ago

khellang commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 https://github.com/httpwg/http11bis/issues/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 commented 6 years ago

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

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

May be interesting to have a look at other platforms

khellang commented 6 years ago

Yeah, looks like this is a good list;

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

khellang commented 6 years ago

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 commented 6 years ago

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

khellang commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

serialseb commented 6 years ago

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 commented 6 years ago

This was superseded by #1043