Skip to content

Clear diagnostic headers on request reuse #113342

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

Merged
merged 7 commits into from
Mar 14, 2025

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Mar 10, 2025

As noted in #86094 (comment), DiagnosticsHandler does not clear manually configured diagnostics headers on a request to avoid interfering with existing user logic. Because of this, if a request is being reused (eg. in Polly retries), DiagnosticsHandler will produce incorrect traces because it reuses the trace state of the first request span for subsequent sends.

However, there is an existing escape mechnism to this rule: in case we detect a redirect we will clear the headers. The presence of redirections is indicated by a flag in HttpRequestMessage.

This PR repurposes the mentioned flag to make sure that headers are also cleared when a request is being reused in a user-supplied handler. This is done by moving the location of flag flipping from RedirectHandler to DiagnosticsHandler while ensuring that user-defined headers are not cleared at the first send attempt.

Fixes #86094.

@Copilot Copilot AI review requested due to automatic review settings March 10, 2025 21:06
@antonfirsov antonfirsov requested a review from MihaZupan March 10, 2025 21:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR refactors how diagnostic headers are cleared when a request is reused, ensuring that user-supplied headers remain intact on the first send while correctly resetting propagation state on retries.

  • Introduces a new test handler (SendMultipleTimesHandler) to mimic request reuse and validate header behavior.
  • Moves the flag handling from RedirectHandler to DiagnosticsHandler and updates HttpRequestMessage flag names accordingly.
  • Removes the RedirectHandler header-clearing call, aligning behavior with the new propagation state flag.

Reviewed Changes

File Description
src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs Added a new test handler and test to validate header reset on reused requests.
src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs Updated condition check and flag marking to use the new propagation state indicator.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs Removed the call to mark the request as redirected to align with the new approach.
src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestMessage.cs Renamed constants and methods from "redirect" to a propagation state context for diagnostics.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs:1523

  • [nitpick] Blocking on async code by calling .Result can lead to potential deadlock issues in certain environments. Consider using .GetAwaiter().GetResult() or refactoring the test to use async/await consistently.
protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken) => SendAsync(request, testAsync: false, cancellationToken).Result;

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Reusing the existing flag for it is a neat way to solve it.

@MihaZupan MihaZupan added this to the 10.0.0 milestone Mar 11, 2025
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.

HttpClient false-negative dependency traces
2 participants