Skip to content

Commit 2470610

Browse files
authored
HttpClientFactory Prevent throwing if ILogger couldn't be created (#90503)
* HttpClientFactory stop cleanup timer on dispose * Remove finalizer * Tweak ThrowIfDisposed * Revert some changes * Fix
1 parent 0c48209 commit 2470610

File tree

2 files changed

+62
-12
lines changed

2 files changed

+62
-12
lines changed

src/libraries/Microsoft.Extensions.Http/src/DefaultHttpClientFactory.cs

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Concurrent;
66
using System.Collections.Generic;
77
using System.Diagnostics;
8+
using System.Diagnostics.CodeAnalysis;
89
using System.Linq;
910
using System.Net.Http;
1011
using System.Threading;
@@ -208,7 +209,7 @@ internal void ExpiryTimer_Tick(object? state)
208209
var expired = new ExpiredHandlerTrackingEntry(active);
209210
_expiredHandlers.Enqueue(expired);
210211

211-
Log.HandlerExpired(_logger.Value, active.Name, active.Lifetime);
212+
Log.HandlerExpired(_logger, active.Name, active.Lifetime);
212213

213214
StartCleanupTimer();
214215
}
@@ -266,7 +267,7 @@ internal void CleanupTimer_Tick()
266267
try
267268
{
268269
int initialCount = _expiredHandlers.Count;
269-
Log.CleanupCycleStart(_logger.Value, initialCount);
270+
Log.CleanupCycleStart(_logger, initialCount);
270271

271272
var stopwatch = ValueStopwatch.StartNew();
272273

@@ -287,7 +288,7 @@ internal void CleanupTimer_Tick()
287288
}
288289
catch (Exception ex)
289290
{
290-
Log.CleanupItemFailed(_logger.Value, entry.Name, ex);
291+
Log.CleanupItemFailed(_logger, entry.Name, ex);
291292
}
292293
}
293294
else
@@ -298,7 +299,7 @@ internal void CleanupTimer_Tick()
298299
}
299300
}
300301

301-
Log.CleanupCycleEnd(_logger.Value, stopwatch.GetElapsedTime(), disposedCount, _expiredHandlers.Count);
302+
Log.CleanupCycleEnd(_logger, stopwatch.GetElapsedTime(), disposedCount, _expiredHandlers.Count);
302303
}
303304
finally
304305
{
@@ -343,24 +344,48 @@ public static class EventIds
343344
"HttpMessageHandler expired after {HandlerLifetime}ms for client '{ClientName}'");
344345

345346

346-
public static void CleanupCycleStart(ILogger logger, int initialCount)
347+
public static void CleanupCycleStart(Lazy<ILogger> loggerLazy, int initialCount)
347348
{
348-
_cleanupCycleStart(logger, initialCount, null);
349+
if (TryGetLogger(loggerLazy, out ILogger? logger))
350+
{
351+
_cleanupCycleStart(logger, initialCount, null);
352+
}
353+
}
354+
355+
public static void CleanupCycleEnd(Lazy<ILogger> loggerLazy, TimeSpan duration, int disposedCount, int finalCount)
356+
{
357+
if (TryGetLogger(loggerLazy, out ILogger? logger))
358+
{
359+
_cleanupCycleEnd(logger, duration.TotalMilliseconds, disposedCount, finalCount, null);
360+
}
349361
}
350362

351-
public static void CleanupCycleEnd(ILogger logger, TimeSpan duration, int disposedCount, int finalCount)
363+
public static void CleanupItemFailed(Lazy<ILogger> loggerLazy, string clientName, Exception exception)
352364
{
353-
_cleanupCycleEnd(logger, duration.TotalMilliseconds, disposedCount, finalCount, null);
365+
if (TryGetLogger(loggerLazy, out ILogger? logger))
366+
{
367+
_cleanupItemFailed(logger, clientName, exception);
368+
}
354369
}
355370

356-
public static void CleanupItemFailed(ILogger logger, string clientName, Exception exception)
371+
public static void HandlerExpired(Lazy<ILogger> loggerLazy, string clientName, TimeSpan lifetime)
357372
{
358-
_cleanupItemFailed(logger, clientName, exception);
373+
if (TryGetLogger(loggerLazy, out ILogger? logger))
374+
{
375+
_handlerExpired(logger, lifetime.TotalMilliseconds, clientName, null);
376+
}
359377
}
360378

361-
public static void HandlerExpired(ILogger logger, string clientName, TimeSpan lifetime)
379+
private static bool TryGetLogger(Lazy<ILogger> loggerLazy, [NotNullWhen(true)] out ILogger? logger)
362380
{
363-
_handlerExpired(logger, lifetime.TotalMilliseconds, clientName, null);
381+
logger = null;
382+
try
383+
{
384+
logger = loggerLazy.Value;
385+
}
386+
catch { } // not throwing in logs
387+
388+
return logger is not null;
364389
}
365390
}
366391
}

src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/DefaultHttpClientFactoryTest.cs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,15 @@
33

44
using System;
55
using System.Collections.Generic;
6+
using System.Linq;
67
using System.Net.Http;
78
using System.Runtime.CompilerServices;
89
using System.Threading;
910
using System.Threading.Tasks;
1011
using Microsoft.Extensions.DependencyInjection;
1112
using Microsoft.Extensions.Logging;
1213
using Microsoft.Extensions.Logging.Abstractions;
14+
using Microsoft.Extensions.Logging.Testing;
1315
using Microsoft.Extensions.Options;
1416
using Moq;
1517
using Moq.Protected;
@@ -508,6 +510,29 @@ private async Task<ExpiredHandlerTrackingEntry> SimulateClientUse_Factory_Cleanu
508510
return cleanupEntry;
509511
}
510512

513+
[Fact]
514+
public void ServiceProviderDispose_DebugLoggingDoesNotThrow()
515+
{
516+
var sink = new TestSink();
517+
518+
var serviceCollection = new ServiceCollection();
519+
serviceCollection.AddLogging();
520+
serviceCollection.AddSingleton<ILoggerFactory>(new TestLoggerFactory(sink, enabled: true));
521+
serviceCollection.AddHttpClient("test");
522+
523+
var serviceProvider = serviceCollection.BuildServiceProvider();
524+
525+
var httpClientFactory = (DefaultHttpClientFactory)serviceProvider.GetRequiredService<IHttpClientFactory>();
526+
_ = httpClientFactory.CreateClient("test");
527+
528+
serviceProvider.Dispose();
529+
530+
httpClientFactory.StartCleanupTimer(); // we need to create a timer instance before triggering cleanup; normally it happens after the first expiry
531+
httpClientFactory.CleanupTimer_Tick(); // trigger cleanup to (try to) write debug logs
532+
// but no log is added, because ILogger couldn't be created
533+
Assert.Equal(0, sink.Writes.Count(w => w.LoggerName == typeof(DefaultHttpClientFactory).FullName));
534+
}
535+
511536
private class TestHttpClientFactory : DefaultHttpClientFactory
512537
{
513538
public TestHttpClientFactory(

0 commit comments

Comments
 (0)