Add trailer support to network responses

[annevk]

Trailer support we can add in the future if this minimal viable
solution is a success:

• Support for synthetic responses.
• Support for (synthetic) requests.
• Trailer headers that have semantics.

Fixes #343 and fixes #34.

[annevk]

Paging @mnot @yutakahirano @tyoshino @louiscryan for review.

[louiscryan]

Looks about right to me.

I wonder what folks think should be permitted in CORS. 'Trailers' is not a mandatory header and is less useful in HTTP2 than it might have been in HTTP1. 'TE: trailers' seems like it has more value however.

[annevk]

You mean for request headers? TE is forbidden at the moment. If we need to start including that I suppose this would be a bigger change and we need some kind of opt-in for that in Request (I doubt user agents would want to always include that for everything). From the discussion at the HTTP workshop that didn't seem to be necessary and the response could just include a trailer if it wanted to.

[yutakahirano]

Ah, I didn't notice this PR when I wrote a comment at #34. Never mind.

I think you need to reject the trailer promise whenever you get an error.
Isn't "response's associated promise" a bit too general for a promise for the trailer?

[annevk]

What kind of error are you thinking about?

[yutakahirano]

Whenever a fetch is terminated after a response is received.

[wenbozhu]

Re: CORS, TE "opt-in" would be desired, as proxies may drop response trailers without such a request header (no?)

Trailer is a response header and there is no harm for the server to always include such a header with Access-Control-Expose-Headers. Discussion or spec clarification may be needed to clarify if UA needs drop trailers that are not specified with Trailer.

[annevk]

@yutakahirano I guess we could do that as it aligns with throwing an error for the body stream, but it also seems fine that trailer just returns an empty header list in that case. Hmm.

[annevk]

Addressed feedback thus far apart from TE opt-in. Not entirely sure what to do with that. Questions I have around that:

1. If the UA did not include that opt-in can the response still contain a trailer legally?
2. Even if it cannot contain it legally, would we still expect it to show up in the API?
3. Should we have an opt-in or just remove the header from the list of forbidden headers? It doesn't seem like the header could be harmful same-origin or cross-origin with CORS, even with arbitrary values.

[mnot]

TE: trailers isn't a terribly useful signal for fetch to generate, because it doesn't tell the server whether individual headers are going to be actually processed (e.g., ETag and Last-Modified for a cache).

What it does help with is determining whether an entire path is compatible with trailers; i.e., whether or not there's a HTTP/1.0 hop. So it make sense for intermediaries to generate it when the downstream path is trailer-safe, or the intermediary buffers responses and sticks them into headers.

[annevk]

Thanks, given httpwg/http-core#18 I suggest we postpone/separate any changes around TE and land this change as-is.

[yutakahirano]

When a response comes from the service worker, no fetch-response-done task is queued, right?

[annevk]

Good point. I should probably move that task queuing to "main fetch" instead somehow.

[annevk]

@yutakahirano does that apply to fetch termination as well?

[annevk]

I moved the task to main fetch. Fetch termination is something we need to sort through further and there's various issues on it already. We can take that up elsewhere.

Anything else?

[annevk]

So I forgot about two things that need to be fixed before landing: 1) Filtering for "no-cors". 2) How this works with CORS.

1. Is easy. It'll just resolve to the empty list.

2. Is hard. I think the easiest v1 would be to also filter it for CORS completely and later we can add a header or some such to reveal them.

[igrigorik]

👍

[annevk]

As a heads up, I plan to land trailer support tomorrow. If you have concerns or would like more time, please leave a comment.

[yutakahirano]

Sorry for the late response. I think Response.clone needs to create a new trailer promise.

[annevk]

Better late than too late. @domenic does the last commit look okay to you? I basically need to resolve the promise of the cloned response when the promise of the original response is fulfilled, but I should probably not reuse the Headers object even though it's immutable (so far we kept the relationships 1:1).

[annevk]

(Also, that was great feedback @yutakahirano, but you probably knew that already. 😊)

[yutakahirano]

LGTM

[annevk]

Thanks everyone for the help. Now it's up to you all to get it tested and implemented. 😊