Skip to content

Correct pending request vs. Dispose behavior in HttpClientConfigFetcher#110

Merged
adams85 merged 1 commit intomasterfrom
fix/configfetcher-dispose
Jun 3, 2025
Merged

Correct pending request vs. Dispose behavior in HttpClientConfigFetcher#110
adams85 merged 1 commit intomasterfrom
fix/configfetcher-dispose

Conversation

@adams85
Copy link
Contributor

@adams85 adams85 commented May 31, 2025

Describe the purpose of your pull request

The implementation of HttpClientConfigFetcher is somewhat sloppy currently because

  • it doesn't correctly handle the race condition between Dispose and FetchAsync, which are not (and can't really be) synchronized,
  • it allows creation of new HttpClient instances after Dispose.

This PR tightens things up in this area.

Related issues (only if applicable)

n/a

How to test? (only if applicable)

n/a

Security (only if applicable)

n/a

Requirement checklist (only if applicable)

  • I have covered the applied changes with automated tests.
  • I have executed the full automated test set against my changes.
  • I have validated my changes against all supported platform versions.
  • I have read and accepted the contribution agreement.

@adams85 adams85 requested a review from a team as a code owner May 31, 2025 14:07
@adams85 adams85 force-pushed the fix/configfetcher-dispose branch from 151d31a to 350aaff Compare May 31, 2025 14:13
@sonarqubecloud
Copy link


private HttpClient EnsureClient(TimeSpan timeout)
{
if (this.httpClient is not { } httpClientObj)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of the pattern matching, but I can live with that.

Copy link
Member

Choose a reason for hiding this comment

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

Why are you not a huge fan of pattern matching?

Copy link
Contributor Author

@adams85 adams85 Jun 2, 2025

Choose a reason for hiding this comment

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

If not pattern matching in general, but the is { }/is not { } syntax might not be the most straightforward, @endret is right about that.

Problem is its usage is pretty widespread already (I found around 15 occurrences).

If you'd like to improve this, I'm open to rewrite these to the more verbose but clearer syntax. But let's do that in a separate PR.

@adams85 adams85 merged commit c39c70c into master Jun 3, 2025
17 checks passed
@adams85 adams85 deleted the fix/configfetcher-dispose branch June 3, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants