Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Content-Length header not always returned when enumerating HttpContentHeaders" #102986

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Jun 3, 2024

Reverts #102416

The change broke outerloop tests in System.Net.Tests.SyncWebClientTest. Seems like it sends bogus Content-Length:
image

cc @pedrobsaila @MihaZupan

@ManickaP
Copy link
Member Author

ManickaP commented Jun 3, 2024

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP requested a review from a team June 3, 2024 11:56
@MihaZupan MihaZupan added this to the 9.0.0 milestone Jun 4, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@MihaZupan
Copy link
Member

@pedrobsaila we decided to revert the change as-is due to concerns of breaking existing code that may be relying on the header not being present.
E.g.

var content = new StringContent("Foo");
content.Headers.Add("Content-Length", "3"); // Throws because the value already exists

Because #16162 mainly impacts diagnostics scenarios (e.g. dumping the current request to the console via ToString), and not the behavior of HttpClient (the header will be sent whether it's initialized early or not), there may still be room for improving those cases by adding more info in HttpRequestMessage.ToString instead of actually modifying the headers collection.

@ManickaP
Copy link
Member Author

ManickaP commented Jun 5, 2024

Failures unrelated, the originally failing tests are not failing anymore.

@ManickaP
Copy link
Member Author

ManickaP commented Jun 5, 2024

/ba-g failures unrelated

@ManickaP ManickaP merged commit b438888 into main Jun 5, 2024
85 of 101 checks passed
@ManickaP ManickaP deleted the revert-102416-16162 branch June 5, 2024 07:16
@michal-zatloukal
Copy link

michal-zatloukal commented Jun 7, 2024

@MihaZupan I strongly disagree that this is only issue for diagnostics. Due to this bug, when I forward HTTP requests on the edge location in our project between our IoT Devices I have to make this:

               private static readonly IReadOnlyList<string> _notForwardedHeaders = new[] { "Host", "Connection", "Keep-Alive", "Upgrade", "Transfer-Encoding" };

...
                var response = await _httpClient.SendAsync(httpRequest, cancellationToken);

                var deviceRequestDuration = Stopwatch.GetElapsedTime(deviceRequestStart);

                var statusCode = (int) response.StatusCode;
                var body = await response.Content.ReadAsByteArrayAsync(cancellationToken);

                var responseHeaders = response.Headers
                    .Where(h => !_notForwardedHeaders.Contains(h.Key))
                    .ToDictionary(h => h.Key, h => string.Join(", ", h.Value));

                // Bug in runtime, 'Content-Length' header not present in the enum, set it manually https://github.com/dotnet/runtime/issues/16162
                if (response.Content.Headers.ContentLength is not null)
                {
                    responseHeaders.Add("Content-Length", response.Content.Headers.ContentLength.Value.ToString());
                }

                // Skip 'Content-Length' anyway for safety (if runtime is fixed)
                foreach (var contentHeader in response.Content.Headers.Where(h => h.Key != "Content-Length"))
                {
                    if (!responseHeaders.TryAdd(contentHeader.Key, string.Join(", ", contentHeader.Value)))
                    {
                        _logger.LogError("Failed to add content header {key} to the response headers.", contentHeader.Key);
                    }
                }

                return new HttpProxyResponse.Successful(statusCode, body, responseHeaders, deviceRequestDuration);

@MihaZupan
Copy link
Member

More discussion about this is on #16162

@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants