I think perhaps we're overcomplicating this.

The simplest approach here seems to be: Make TrailingHeaders work just like Headers, i.e. lazily instantiated, no setter.

This approach avoids handler lock-in, and has no additional overhead if you don't use trailers (the 99% case).

What it doesn't address is:
(1) If you do care about trailers, but there aren't actually any trailers on the response, then this will incur an additional allocation of an empty headers collection for TrailingHeaders. I'm not convinced this matters. This is a very uncommon case, and we already allocate quite a bit in this scenario anyway. One additional empty collection doesn't seem like it matters much.
(2) How can a user know when the trailers have been processed and made available? First of all, in the default case where the response is buffered, this isn't an issue, because trailers will always be made available immediately. It's only the non-buffered case where this is an issue. For this case, I think we can simply say that trailers are available once you have read to the end of the response stream. This seems straightforward and intuitive.

So, my suggestion is, let's just do the simple thing here.

Sounds workable.

For this case, I think we can simply say that trailers are available once you have read to the end of the response stream. This seems straightforward and intuitive.

What happens if someone accesses the collection before then? Presumably it would get a collection that could potentially be filled in/ mutated concurrently, so we'd need to be very clear that you couldn't actually access the collection until the stream ended, or else race conditions.

Other than that I'm fine with it. My primary concern is not negatively impacting the 99.9% where there are no trailers and the consumer doesn't care about trailers, and in such cases I would be concerned about allocating an additional collection. We just need to be sure then we don't accidentally force it into existence unnecessarily.

What happens if someone accesses the collection before then? Presumably it would get a collection that could potentially be filled in/ mutated concurrently, so we'd need to be very clear that you couldn't actually access the collection until the stream ended, or else race conditions.

This seems like a recipe for disaster. We should not give out a collection but then hope that someone doesn't modify it while our HTTP stack is trying to add elements to it.

If the headers aren't ready, we need to give out something immutable or NULL etc.

HttpResponseMessage and its associated objects are not thread safe and should not be used in a concurrent manner.

I'm pretty sure there are plenty of ways you could get into trouble today if you do this. For example, if you try to read on the response stream from multiple threads concurrently, very bad things could happen.

For example, if you try to read on the response stream from multiple threads concurrently, very bad things could happen.

This specific type of thing should already be protected. Most of our network streams public API surface for Read/Write protect against starting a new Read/Write while one is in progress. They usually will throw an InvalidOperationException in those cases.

To clarify a bit:

What happens if someone accesses the collection before then? Presumably it would get a collection that could potentially be filled in/ mutated concurrently, so we'd need to be very clear that you couldn't actually access the collection until the stream ended, or else race conditions.

We should ensure that the collection is never mutated concurrently. Rather, it is mutated at a very specific point, which is when the caller reads the response stream and there is no more data, so read returns 0 bytes to indicate EOF. I believe we've already agreed this is when trailers should be made available, independent of how they are actually surfaced.

If we do this, then it's fine to retrieve and access the collection either before or after receiving EOF. You just won't see any actual trailing headers until after you receive the EOF.

For HTTP/1.1, implementing this as described above is straightforward. For HTTP2, it's a little trickier because we can receive the trailing HEADERS frame concurrently to however the user is actually accessing the HttpResponseMessage object. That means we will probably need to store the received trailing headers on the Http2Stream initially, and only add them to the actual TraiingHeaders collection when we are returning EOF. But we need to do something like this anyway to implement the correct behavior for when trailing headers become available.

This specific type of thing should already be protected. Most of our network streams public API surface for Read/Write protect against starting a new Read/Write while one is in progress.

As far as I'm aware, we don't do this for request/response streams in SocketsHttpHandler.

But regardless, I don't think HttpResponseMessage is intended to support concurrent access and I don't think we need to or want to provide guarantees here.

But regardless, I don't think HttpResponseMessage is intended to support concurrent access and I don't think we need to or want to provide guarantees here.

We will always return the HttpResponseMessage only after we have the Headers ready - we don't need to concern about the concurrent access since we have done with Headers collection. IMHO, it's consistent.

However, TrailingHeaders is not, and I think consistency is required - we should not return a half-done and modifiable collection to user. Therefore, as Stephen and Geoff have mentioned, two approaches to pick from:

a) Current pattern: use the setter. I can make it internal/private for now, and we can discuss later if we want to make setter public for all related properties.

b) Add an additional TrailingHeadersStatus property, so that users will know what's the current stage in parsing trailers. Notice: they still can ignore this new property, and nothing will prevent them from partying on the TrailingHeaders.

I vote for a) for simplicity and additional protection for concurrency. Thoughts?
@dotnet/ncl

I think @geoffkizer had it nailed down pretty well. Make the headers lazy initialized without a setter like all the others and only populate them during the final Stream.ReadAsync. That way you're doing it on the application's thread and know the application isn't (shouldn't) be touching any of the other objects at the same time.

and only populate them during the final Stream.ReadAsync

Oh, sorry I missed that part. Sounds workable, let me fix in next commit.

That way you're doing it on the application's thread

I'm not sure what that means, since the code could be:

var headers = resp.TrailingHeaders;
var read = stream.ReadAsync(...);
foreach (var header in headers) { ... }
await read;

so we'll need to be very clear about what the guarantees are. I suggest we explicitly say that consuming code should never access the collection property until EOF.

We should also make sure that we don't inadvertently add to the collection while consuming trailing headers while draining a connection that's being returned back to the pool. Such trailers should be ignored.

we'll need to be very clear about what the guarantees are. I suggest we explicitly say that consuming code should never access the collection property until EOF.

I don't think we need to be that strict (though it doesn't hurt to say that). The code you posted should work fine in this approach.

I will need this PR in for HTTP/2 work, PTAL, thanks.

/cc: @dotnet/ncl

your feedback should be covered @stephentoub. Please take another look when you get chance.