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@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel