[PATCH 1 of 2] HTTP: add support for trailers in HTTP responses

[shuxinyang]

Hi, Piotr:

      I took some time to read your code, I believe I fully understand 
your code at this moment.
My implementation is slightly differently from yours, and we have 
different use-cases.

      Here is my $0.02 value of comment to your code (cosmetic wise):
      - the verb "support" in "trailer support" in too general. The 
trailer-support in my mind include
        a long list of features (see bellow)

      - the "get" in "ngx_http_chunked_get_trailers" is sort of 
misleading. The "get" here actually
        means "generate". I thought you are looking for a existing buffer.

   Now go back to the long list of trailer support,  I believe it needs 
to cover at least:
     1) generate trailer headers and sent to downstream/visitor as you 
just did
     2) reverse proxy propagate trailer headers from upstream, which needs
         2.0 parse the incoming trailer header, both considering the 
body is buffered and not buffered,
         2.1. make the incoming trailer accessible via variable after 
content phase,
         2.2. if response body is is not buffered, combine incoming 
trailer header with the generated headers.
         2.3. convert incoming trailer to regular header if response 
body is buffered

    3|4|..|n) other features.

    I think 2.x is much harder than 1). I only implement 1) and 2.0) and 
2.1). While implement 2.1), I left some
room for 2.2 and 2.3 for the future. Now that you have plan for 2.x as 
your email suggests, wouldn't it
be nice to submit these code first, and then go ahead with the code of 
1). I believe to support 2.2 or 2.3,
your existing code in chunk module needs lots of change.

Thanks
Shuxin


On 06/27/2016 12:14 PM, Piotr Sikora wrote:
> Hey Shuxin,
>
>>    As far as I can understand, your change is just to add trailer headers
>> (not including the part that paring incoming
>> trailer header from upstream, or merge the incoming trailer and generated
>> trailer). If that is correct, you just need
>> to add "trailer: hdr1,hdr2...  hdrn" to the out-headers.
> Did you look at both patches I've sent?
> http://mailman.nginx.org/pipermail/nginx-devel/2016-June/008429.html
> http://mailman.nginx.org/pipermail/nginx-devel/2016-June/008430.html
>
> Because they cover much more than just adding "Trailer" header.
>
>> TE is for something
>> else as Maxim pointed out,
>> and adding this header can be done in chunked-filter-module as well.
> "TE" is a _request_ header, so I don't see how adding it to response
> is relevant here.
>
> And yes, "TE" header could be parsed in chunked filter, but it's IMHO
> wrong place to do it, since you need to parse this header in HTTP/2
> requests as well.
>
>>    My previous implementation of generating trailer header is completely done
>> in chunk-module.  Later on, I change
>> my mind, and add a standalone module along with minor change to configure
>> script.
> Does you implementation support trailers in HTTP/2 as well?
>
> Best regards,
> Piotr Sikora
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel