[HttpClientFactory] Revert workaround for empty name HttpClient fallback #106269
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
CarnaViire
changed the title
| using System.Net.Http; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Extensions.DependencyInjection | ||
| { | ||
| public partial class HttpClientKeyedRegistrationTest | ||
| { | ||
| [Fact] | ||
| public void AddAsKeyed_EmptyNameHttpClientUpdated() // test for workaround for https://github.com/dotnet/runtime/issues/102654 |
There was a problem hiding this comment.
Is there no value in keeping these tests?
There was a problem hiding this comment.
None -- they were testing specifically the workaround logic that was removed.
src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
| /// WARNING: Registering the client as a keyed <see cref="ServiceLifetime.Transient"/> service will lead to the <see cref="HttpClient"/> and <see cref="HttpMessageHandler"/> | ||
| /// instances being captured by DI as both implement <see cref="IDisposable"/>. This might lead to memory leaks if the client is resolved multiple times within a | ||
| /// <see cref="ServiceLifetime.Singleton"/> service. |
There was a problem hiding this comment.
This seems pretty sad.
However, this isn't specific to the IHttpClientFactory, and the same issue exists with any disposable service, right? It's just a new foot gun for us since we couldn't be transient before?
There was a problem hiding this comment.
But there is hope that this might be finally addressed (in some way) in 10.0 (this was discussed during API review, as it was one of the main blockers for keyed-by-default). Also see #89755 (comment). Tracking issue: #36461 but we'd need to also create an additional one for per-service-descriptor opt-out.
…n/HttpClientBuilderExtensions.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
Revert the workaround from e958285, as #102654 was recently fixed.
Also adds the missing tripleslash docs.