Content-Length header not always returned when enumerating HttpContentHeaders

[pedrobsaila]

Fixes #16162

[MihaZupan]

Forcing the computation in the constructor is too early, derived types may not even get a chance to initialize.

Consider this example. With this change, TryComputeLength would be called before _path is set, causing an argument exception to be thrown that would then be ignored. This could mean TryComputeLength may be called more than once, which wouldn't happen before.

public sealed class FileHttpContent : HttpContent
{
    private readonly string _path;
    
    public FileHttpContent(string path)
    {
        Headers.Add("X-File-Name", Uri.EscapeDataString(Path.GetFileName(path)));
        _path = path;
    }
    
    protected override Task SerializeToStreamAsync(Stream stream, TransportContext context) =>
        SerializeToStreamAsync(stream, context, CancellationToken.None);

    protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken)
    {
        await using FileStream fs = File.OpenRead(_path);
        await fs.CopyToAsync(stream, cancellationToken);
    }

    protected override bool TryComputeLength(out long length)
    {
        length = new FileInfo(_path).Length;
        return true;
    }
}

If we want to force the value to be available during enumeration, we should force its creation when trying to enumerate and not before.

[pedrobsaila]

That means I need to overload GetEnumerator in HttpContentHeaders. Will this need an api review ?

[MihaZupan]

These are all internal implementation details, we shouldn't need to make API changes.
For example, the GetEnumerator calls in HttpHeaders and HttpHeadersNonValidated could do the necessary type checks.

[pedrobsaila]

Sorry if I'm asking too much questions, but HttpHeaders & HttpHeadersNonValidated do not know about HttpContent nor its length. I don't see how updating GetEnumerator in these two classes would help.

[MihaZupan]

What I mean is that where the HttpHeaders.GetEnumerator is currently

public IEnumerator<KeyValuePair<string, IEnumerable<string>>> GetEnumerator() =>
    GetEnumeratorCore();

it could instead be

public IEnumerator<KeyValuePair<string, IEnumerable<string>>> GetEnumerator()
{
    if (this is HttpContentHeaders contentHeaders)
    {
        _ = contentHeaders.ContentLength;
    }

    return GetEnumeratorCore();
}

[MihaZupan]

Thank you for looking into these issues.

Thinking about it a bit more, this would still run into the issue mentioned in #102416 (comment) if the implementation tried enumerating the headers from its constructor (e.g. replace Headers.Add with Headers.ToString()).
While I doubt that's all that common, it feels too risky to just break.

We could go with just A) mentioned here: #16162 (comment)
That is, only fix the issue for built-in types.

[pedrobsaila]

Thinking about it a bit more, this would still run into the issue mentioned in #102416 (comment) if the implementation tried enumerating the headers from its constructor (e.g. replace Headers.Add with Headers.ToString()).

The test System.Net.Http.Tests.HttpContentHeadersTest.Should_LazyLoad_ContentLengthWhenEnumeratingAndToString covers this case and passes without doing Headers.Add

[pedrobsaila]

We could go with just A) mentioned here: #16162 (comment)

Do I make public HttpContent.Headers property virtual and override it in ByteArrayContent, StringContent, and FormUrlEncodedContent by calling TryLoadContentLength ?

[MihaZupan]

I was thinking it may be as simple as Headers.ContentLength = count; in ByteArrayContent / StreamContent's ctor

[MihaZupan]

Thanks.

ReadOnlyMemoryContent looks like another place where this could be applied

[BrennanConroy]

Should HttpClient be removing/throwing if TransferEncoding: chunked is also set?
https://www.rfc-editor.org/rfc/rfc9112#section-6.1-15

A server MAY reject a request that contains both Content-Length and Transfer-Encoding or process such a request in accordance with the Transfer-Encoding alone. Regardless, the server MUST close the connection after responding to such a request to avoid the potential attacks.

https://www.rfc-editor.org/rfc/rfc9112#section-6.2-2

A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field.

Noticed this because one of our tests is failing due to this update and it is also explicitly setting chunked transfer encoding
https://github.com/dotnet/aspnetcore/blob/7e6c237285cd12e93b5e882d835eb8fcec1c8ec7/src/Hosting/TestHost/test/ClientHandlerTests.cs#L183

[MihaZupan]

We do that validation at the SocketsHttpHandler layer:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

Lines 618 to 635 in f6701e5

	// Add headers to define content transfer, if not present
	if (request.HasHeaders && request.Headers.TransferEncodingChunked.GetValueOrDefault())
	{
	if (request.Content == null)
	{
	return new HttpRequestException(SR.net_http_client_execution_error,
	new InvalidOperationException(SR.net_http_chunked_not_allowed_with_empty_content));
	}
	// Since the user explicitly set TransferEncodingChunked to true, we need to remove
	// the Content-Length header if present, as sending both is invalid.
	request.Content.Headers.ContentLength = null;
	}
	else if (request.Content != null && request.Content.Headers.ContentLength == null)
	{
	// We have content, but neither Transfer-Encoding nor Content-Length is set.
	request.Headers.TransferEncodingChunked = true;
	}

I'm guessing your test is skipping that since you have a custom handler?

[BrennanConroy]

Ah ok, good to know