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
Rename HttpClientFactory
to WasabiHttpClientFactory
and introduce CreateLongLivedHttpClient
helper
#11307
Rename HttpClientFactory
to WasabiHttpClientFactory
and introduce CreateLongLivedHttpClient
helper
#11307
Conversation
@lontivero @molnard concept ACK? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK on RegTest, CV also works.
Don't be afraid of the 26 file changes, most of them are just renamings. Code LGTM.
cack |
/// <remarks>Created HTTP client handles correctly DNS changes.</remarks> | ||
/// <seealso href="https://learn.microsoft.com/en-us/dotnet/core/extensions/httpclient-factory"/> | ||
[SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "HTTP client will dispose the handler correctly.")] | ||
public static HttpClient CreateLongLivedHttpClient(TimeSpan? pooledConnectionLifetime = null, DecompressionMethods? automaticDecompression = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method can be even in a different class. I have experimented with this but I can't seem to find a nicer/better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Basically no change here in behavior. Except that pooledConnectionLifetime by default was InfiniteTimeSpan?
Now it is 5 minutes, right?
The PR is designed as just refactoring. There should be no change in behavior. We had that 5-minute timeout here: https://github.com/zkSNACKs/WalletWasabi/pull/11307/files#diff-a0fad88b3a26fafa04bbd69a412b28ad930c02cbed0ae3b83026da06122006baL26. But we can also align with .NET where the timeout is 2 minutes (if I remember correctly). |
Alternative to #10569
Notes:
HttpClientFactory
is renamed toWasabiHttpClientFactory
to avoid confusion withIHttpClientFactory
in .NET.WasabiHttpClientFactory.CreateLongLivedHttpClient
allows to createHttpClient
instances that can live for long-time (as DNS is properly updated).Global
was done to use the newWasabiHttpClientFactory.CreateLongLivedHttpClient
helper method.Review is easy commit by commit. Each commit can be a standalone PR basically if needed.