Skip to content

Commit 543c80f

Browse files
authored
Removes exception throwing during shutdown of dynamic instrumentation (#7375)
## Summary of changes Removes exception throwing in "success" path, even when DI is disabled ## Reason for change #7304 introduced a bunch of changes, but the use of `CancellationTokenSource` resulted in exceptions being thrown in the "happy" shutdown path, which can cause crashes in buggy versions of .NET (i.e. all of them, currently) ## Implementation details Replace usages of `TaskCompletionSource<bool>` with `CancellationTokenSource` ## Test coverage Covered by existing - checked the execution tests to make sure the exception count has gone back own. ## Other details Discovered some additional issues that need to be addressed I think. Most importantly, `SafeDisposal` looks like a band-aid due to unclear lifetime management. We should refactor the code to not require it i.e. `Disposing` types should be safe and should not throw (regardless of whether we catch the exception). Additionally, this looks like it does quite a lot of work even when DI is disabled. I would suggest we refactor it so that it doesn't do a bunch of work in cases where it's never enabled.
1 parent a91ff29 commit 543c80f

File tree

2 files changed

+51
-45
lines changed

2 files changed

+51
-45
lines changed

tracer/src/Datadog.Trace/Debugger/DebuggerManager.cs

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,19 +38,16 @@ internal class DebuggerManager
3838

3939
private static volatile bool _discoveryServiceReady;
4040
private readonly SemaphoreSlim _semaphore;
41-
private readonly CancellationTokenSource _cancellationToken;
42-
private volatile bool _isShuttingDown;
41+
private readonly TaskCompletionSource<bool> _processExit = new(TaskCreationOptions.RunContinuationsAsynchronously);
4342
private int _initialized;
4443

4544
private DebuggerManager(DebuggerSettings debuggerSettings, ExceptionReplaySettings exceptionReplaySettings)
4645
{
4746
_initialized = 0;
4847
_discoveryServiceReady = false;
49-
_isShuttingDown = false;
5048
_semaphore = new SemaphoreSlim(1, 1);
5149
DebuggerSettings = debuggerSettings;
5250
ExceptionReplaySettings = exceptionReplaySettings;
53-
_cancellationToken = new CancellationTokenSource();
5451
ServiceName = string.Empty;
5552
}
5653

@@ -70,7 +67,7 @@ private DebuggerManager(DebuggerSettings debuggerSettings, ExceptionReplaySettin
7067

7168
internal string ServiceName { get; private set; }
7269

73-
private async Task<bool> WaitForDiscoveryServiceAsync(IDiscoveryService discoveryService, CancellationToken cancellationToken)
70+
private async Task<bool> WaitForDiscoveryServiceAsync(IDiscoveryService discoveryService, Task shutdownTask)
7471
{
7572
if (_discoveryServiceReady)
7673
{
@@ -79,10 +76,10 @@ private async Task<bool> WaitForDiscoveryServiceAsync(IDiscoveryService discover
7976

8077
var tc = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
8178

82-
using var registration = cancellationToken.Register(() => tc.TrySetResult(false));
83-
8479
discoveryService.SubscribeToChanges(Callback);
85-
_discoveryServiceReady = await tc.Task.ConfigureAwait(false);
80+
var completedTask = await Task.WhenAny(shutdownTask, tc.Task).ConfigureAwait(false);
81+
82+
_discoveryServiceReady = completedTask == tc.Task;
8683
return _discoveryServiceReady;
8784

8885
void Callback(AgentConfiguration x)
@@ -209,7 +206,7 @@ private async Task SetDynamicInstrumentationState(TracerSettings tracerSettings)
209206
var sw = Stopwatch.StartNew();
210207
if (!_discoveryServiceReady)
211208
{
212-
var isDiscoverySuccessful = await WaitForDiscoveryServiceAsync(discoveryService, _cancellationToken.Token).ConfigureAwait(false);
209+
var isDiscoverySuccessful = await WaitForDiscoveryServiceAsync(discoveryService, _processExit.Task).ConfigureAwait(false);
213210
if (!isDiscoverySuccessful)
214211
{
215212
Log.Warning("Discovery service is not ready, Dynamic Instrumentation will not be initialized.");
@@ -238,7 +235,7 @@ internal Task UpdateConfiguration(TracerSettings tracerSettings, DebuggerSetting
238235

239236
private async Task UpdateProductsState(TracerSettings tracerSettings, DebuggerSettings newDebuggerSettings)
240237
{
241-
if (_isShuttingDown)
238+
if (_processExit.Task.IsCompleted)
242239
{
243240
return;
244241
}
@@ -248,8 +245,20 @@ private async Task UpdateProductsState(TracerSettings tracerSettings, DebuggerSe
248245
bool semaphoreAcquired = false;
249246
try
250247
{
251-
semaphoreAcquired = await _semaphore.WaitAsync(TimeSpan.FromSeconds(30), _cancellationToken.Token).ConfigureAwait(false);
252-
if (!semaphoreAcquired || _isShuttingDown)
248+
var attemptsRemaining = 6; // 5*6 = 30s timeout
249+
while (!_processExit.Task.IsCompleted && !semaphoreAcquired && attemptsRemaining > 0)
250+
{
251+
attemptsRemaining--;
252+
semaphoreAcquired = await _semaphore.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false);
253+
}
254+
255+
if (_processExit.Task.IsCompleted)
256+
{
257+
Log.Debug("Skipping update debugger state due to process exit");
258+
return;
259+
}
260+
261+
if (!semaphoreAcquired)
253262
{
254263
Log.Debug("Skipping update debugger state due to semaphore timed out");
255264
return;
@@ -345,19 +354,22 @@ private async Task InitializeSymbolUploader(TracerSettings tracerSettings)
345354

346355
private void ShutdownTasks(Exception? ex)
347356
{
348-
_isShuttingDown = true;
357+
if (_processExit.Task.IsCompleted)
358+
{
359+
return;
360+
}
361+
362+
_processExit.TrySetResult(true);
349363

350364
if (ex != null)
351365
{
352366
Log.Debug(ex, "Shutdown task for DebuggerManager is running with exception");
353367
}
354368

355369
SafeDisposal.New()
356-
.Execute(() => _cancellationToken.Cancel(), "cancelling DebuggerManager operations")
357370
.Add(DynamicInstrumentation)
358371
.Add(ExceptionReplay)
359372
.Add(SymbolsUploader)
360-
.Add(_cancellationToken)
361373
.Add(_semaphore)
362374
.DisposeAll();
363375
}

tracer/src/Datadog.Trace/Debugger/Symbols/SymbolsUploader.cs

Lines changed: 24 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,13 @@ internal class SymbolsUploader : IDebuggerUploader
4242
private readonly ImmutableHashSet<string> _symDb3rdPartyIncludes;
4343
private readonly ImmutableHashSet<string> _symDb3rdPartyExcludes;
4444
private readonly long _thresholdInBytes;
45-
private readonly CancellationTokenSource _cancellationToken;
45+
private readonly TaskCompletionSource<bool> _processExit = new(TaskCreationOptions.RunContinuationsAsynchronously);
4646
private readonly IBatchUploadApi _api;
4747
private readonly IRcmSubscriptionManager _subscriptionManager;
4848
private readonly ISubscription _subscription;
4949
private readonly object _disposeLock = new();
50+
private readonly IDiscoveryService _discoveryService;
5051
private volatile bool _disposed = false;
51-
private IDiscoveryService? _discoveryService;
5252
private byte[]? _payload;
5353
private string? _symDbEndpoint;
5454
private bool _isSymDbEnabled;
@@ -74,7 +74,6 @@ private SymbolsUploader(
7474
_thresholdInBytes = settings.SymbolDatabaseBatchSizeInBytes;
7575
_symDb3rdPartyIncludes = settings.SymDbThirdPartyDetectionIncludes;
7676
_symDb3rdPartyExcludes = settings.SymDbThirdPartyDetectionExcludes;
77-
_cancellationToken = new CancellationTokenSource();
7877
_jsonSerializerSettings = new JsonSerializerSettings { NullValueHandling = NullValueHandling.Ignore };
7978
_discoveryService.SubscribeToChanges(ConfigurationChanged);
8079
_subscription = new Subscription(Callback, RcmProducts.LiveDebuggingSymbolDb);
@@ -102,7 +101,7 @@ into rawFile
102101
{
103102
_isSymDbEnabled = false;
104103
UnRegisterToAssemblyLoadEvent();
105-
_cancellationToken.Cancel(false);
104+
_processExit.TrySetResult(true);
106105
}
107106

108107
return Array.Empty<ApplyDetails>();
@@ -118,8 +117,7 @@ private void ConfigurationChanged(AgentConfiguration configuration)
118117

119118
_symDbEndpoint = configuration.SymbolDbEndpoint;
120119
_discoveryServiceSemaphore.Release(1);
121-
_discoveryService!.RemoveSubscription(ConfigurationChanged);
122-
_discoveryService = null;
120+
_discoveryService.RemoveSubscription(ConfigurationChanged);
123121
}
124122

125123
public static IDebuggerUploader Create(IBatchUploadApi api, IDiscoveryService discoveryService, IRcmSubscriptionManager remoteConfigurationManager, TracerSettings tracerSettings, DebuggerSettings settings, string serviceName)
@@ -167,10 +165,13 @@ private async Task ProcessItemAsync(Assembly assembly)
167165
bool semaphoreAcquired = false;
168166
try
169167
{
170-
await _assemblySemaphore.WaitAsync(_cancellationToken.Token).ConfigureAwait(false);
171-
semaphoreAcquired = true;
168+
var acquireTimeout = TimeSpan.FromSeconds(5);
169+
while (!_processExit.Task.IsCompleted && !semaphoreAcquired)
170+
{
171+
semaphoreAcquired = await _assemblySemaphore.WaitAsync(acquireTimeout).ConfigureAwait(false);
172+
}
172173

173-
if (!_isSymDbEnabled || _cancellationToken.IsCancellationRequested || _disposed)
174+
if (!_isSymDbEnabled || _processExit.Task.IsCompleted || _disposed)
174175
{
175176
return;
176177
}
@@ -392,7 +393,6 @@ public async Task StartFlushingAsync()
392393

393394
if (await WaitForEnablementAsync().ConfigureAwait(false) == false)
394395
{
395-
Log.Information("This can happen when the service is shut down");
396396
return;
397397
}
398398

@@ -413,7 +413,7 @@ private async Task<bool> WaitForDiscoveryServiceAsync()
413413
return true;
414414
}
415415

416-
if (_disposed || _cancellationToken.IsCancellationRequested)
416+
if (_disposed || _processExit.Task.IsCompleted)
417417
{
418418
return false;
419419
}
@@ -422,10 +422,12 @@ private async Task<bool> WaitForDiscoveryServiceAsync()
422422

423423
try
424424
{
425-
await _discoveryServiceSemaphore.WaitAsync(_cancellationToken.Token).ConfigureAwait(false);
426-
_discoveryServiceSemaphore.Dispose();
425+
var completedTask = await Task.WhenAny(
426+
_discoveryServiceSemaphore.WaitAsync(),
427+
_processExit.Task)
428+
.ConfigureAwait(false);
427429

428-
if (_cancellationToken.IsCancellationRequested || _disposed)
430+
if (completedTask == _processExit.Task || _disposed)
429431
{
430432
return false;
431433
}
@@ -444,7 +446,7 @@ private async Task<bool> WaitForDiscoveryServiceAsync()
444446

445447
private async Task<bool> WaitForEnablementAsync()
446448
{
447-
if (_disposed || _cancellationToken.IsCancellationRequested)
449+
if (_disposed || _processExit.Task.IsCompleted)
448450
{
449451
return false;
450452
}
@@ -453,8 +455,12 @@ private async Task<bool> WaitForEnablementAsync()
453455

454456
try
455457
{
456-
await _enablementSemaphore.WaitAsync(_cancellationToken.Token).ConfigureAwait(false);
457-
if (_cancellationToken.IsCancellationRequested || _disposed)
458+
var completedTask = await Task.WhenAny(
459+
_enablementSemaphore.WaitAsync(),
460+
_processExit.Task)
461+
.ConfigureAwait(false);
462+
463+
if (completedTask == _processExit.Task || _disposed)
458464
{
459465
return false;
460466
}
@@ -488,19 +494,7 @@ public void Dispose()
488494
_disposed = true;
489495
_subscriptionManager.Unsubscribe(_subscription);
490496

491-
try
492-
{
493-
if (!_cancellationToken.IsCancellationRequested)
494-
{
495-
_cancellationToken.Cancel();
496-
}
497-
}
498-
catch (ObjectDisposedException)
499-
{
500-
// Already disposed, ignore
501-
}
502-
503-
_cancellationToken.Dispose();
497+
_processExit.TrySetResult(true);
504498
_assemblySemaphore.Dispose();
505499
_enablementSemaphore.Dispose();
506500
_discoveryServiceSemaphore.Dispose();

0 commit comments

Comments
 (0)