Skip to content

Commit 1d6bfc7

Browse files
dudikeletidudikeleti
committed
fix race during di state change
1 parent 51a75d1 commit 1d6bfc7

File tree

1 file changed

+136
-64
lines changed

1 file changed

+136
-64
lines changed

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

Lines changed: 136 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ namespace Datadog.Trace.Debugger
2626
internal class DebuggerManager
2727
{
2828
private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(DebuggerManager));
29+
private static readonly TimeSpan DebounceDelay = TimeSpan.FromMilliseconds(250);
30+
private static readonly TimeSpan EndpointTimeout = TimeSpan.FromMinutes(5);
2931

3032
private static readonly Lazy<DebuggerManager> _lazyInstance =
3133
new(
@@ -41,6 +43,7 @@ internal class DebuggerManager
4143
private int _initialized;
4244
private CancellationTokenSource? _diDebounceCts;
4345
private volatile DynamicInstrumentation? _dynamicInstrumentation;
46+
private int _diState; // 0 = disabled, 1 = initializing, 2 = initialized
4447

4548
private DebuggerManager(DebuggerSettings debuggerSettings, ExceptionReplaySettings exceptionReplaySettings)
4649
{
@@ -50,7 +53,7 @@ private DebuggerManager(DebuggerSettings debuggerSettings, ExceptionReplaySettin
5053
ExceptionReplaySettings = exceptionReplaySettings;
5154
ServiceName = string.Empty;
5255
_syncLock = new();
53-
_diDebounceDelay = TimeSpan.FromMilliseconds(250);
56+
_diDebounceDelay = DebounceDelay;
5457
_processExit = new(TaskCreationOptions.RunContinuationsAsynchronously);
5558
}
5659

@@ -65,7 +68,7 @@ internal DynamicInstrumentation? DynamicInstrumentation
6568
get
6669
{
6770
var instance = _dynamicInstrumentation;
68-
return instance?.IsDisposed == false ? instance : null;
71+
return instance is { IsDisposed: false, IsInitialized: true } ? instance : null;
6972
}
7073

7174
private set => _dynamicInstrumentation = value;
@@ -104,12 +107,11 @@ private async Task<bool> WaitForDebuggerEndpointAsync(IDiscoveryService discover
104107

105108
try
106109
{
107-
var debuggerEndpointTimeout = TimeSpan.FromMinutes(5);
108-
var timeoutTask = Task.Delay(debuggerEndpointTimeout);
110+
var timeoutTask = Task.Delay(EndpointTimeout);
109111
var completedTask = await Task.WhenAny(debuggerEndpointTcs.Task, timeoutTask, processExitTask).ConfigureAwait(false);
110112
if (completedTask == timeoutTask)
111113
{
112-
Log.Warning("Debugger endpoint is not available after waiting {Timeout} seconds.", debuggerEndpointTimeout.TotalSeconds);
114+
Log.Warning("Debugger endpoint is not available after waiting {Timeout} seconds.", EndpointTimeout.TotalSeconds);
113115
return false;
114116
}
115117

@@ -320,41 +322,65 @@ private async Task ScheduleStartDynamicInstrumentation(TracerSettings tracerSett
320322
return;
321323
}
322324

323-
// Only proceed if state actually needs to change
324-
if (ShouldSkipUpdate(debuggerSettings.DynamicInstrumentationEnabled, DynamicInstrumentation != null))
325+
var requested = debuggerSettings.DynamicInstrumentationEnabled;
326+
var current = DynamicInstrumentation != null;
327+
var state = Volatile.Read(ref _diState);
328+
329+
if (!requested && (current || state == 1))
325330
{
331+
// cancel any pending enable and disable immediately
332+
var currentCts = Interlocked.Exchange(ref _diDebounceCts, null);
333+
try
334+
{
335+
currentCts?.Cancel();
336+
}
337+
catch (ObjectDisposedException)
338+
{
339+
// ignore
340+
}
341+
342+
SafeDisposal.TryDispose(currentCts);
343+
DisableDynamicInstrumentation(debuggerSettings.DynamicSettings.DynamicInstrumentationEnabled == false);
326344
return;
327345
}
328346

329-
// Cancel any pending operation and start new one
347+
if (ShouldSkipUpdate(requested, current))
348+
{
349+
return;
350+
}
351+
352+
// cancel any pending operation and start new one
330353
var cts = new CancellationTokenSource();
331354
var prevCts = Interlocked.Exchange(ref _diDebounceCts, cts);
332-
Log.Debug("Is previous Dynamic Instrumentation update request exist? {Value}", prevCts != null);
355+
Log.Debug("Previous DI update request exists: {Value}", prevCts != null);
333356

334357
try
335358
{
336359
prevCts?.Cancel();
337360
}
338361
catch (ObjectDisposedException)
339362
{
340-
// Previous token was already disposed, ignore
363+
// ignore
341364
}
342365

343366
try
344367
{
345368
Log.Debug("Waiting {Timeout}ms before updating Dynamic Instrumentation", _diDebounceDelay.TotalMilliseconds);
346369
await Task.Delay(_diDebounceDelay, cts.Token).ConfigureAwait(false);
347370

348-
// Re-check if state change is still needed after debounce
349-
if (!cts.IsCancellationRequested)
371+
if (_processExit.Task.IsCompleted || cts.IsCancellationRequested)
350372
{
351-
if (ShouldSkipUpdate(DebuggerSettings.DynamicInstrumentationEnabled, DynamicInstrumentation != null))
352-
{
353-
return;
354-
}
373+
return;
374+
}
355375

356-
await SetDynamicInstrumentationStateAsync(tracerSettings, DebuggerSettings, cts.Token).ConfigureAwait(false);
376+
var requested2 = DebuggerSettings.DynamicInstrumentationEnabled;
377+
var current2 = DynamicInstrumentation != null;
378+
if (ShouldSkipUpdate(requested2, current2))
379+
{
380+
return;
357381
}
382+
383+
await SetDynamicInstrumentationStateAsync(tracerSettings, cts.Token).ConfigureAwait(false);
358384
}
359385
catch (OperationCanceledException)
360386
{
@@ -372,7 +398,8 @@ private async Task ScheduleStartDynamicInstrumentation(TracerSettings tracerSett
372398

373399
bool ShouldSkipUpdate(bool requested, bool current)
374400
{
375-
if (requested == current)
401+
// no change required AND no init in progress
402+
if ((requested == current) && Volatile.Read(ref _diState) == 0)
376403
{
377404
Log.Debug("Skip update Dynamic Instrumentation. Requested is {Requested}, Current is {Current}", requested, current);
378405
return true;
@@ -382,7 +409,7 @@ bool ShouldSkipUpdate(bool requested, bool current)
382409
}
383410
}
384411

385-
private async Task SetDynamicInstrumentationStateAsync(TracerSettings tracerSettings, DebuggerSettings debuggerSettings, CancellationToken cancellationToken)
412+
private async Task SetDynamicInstrumentationStateAsync(TracerSettings tracerSettings, CancellationToken cancellationToken)
386413
{
387414
try
388415
{
@@ -391,21 +418,18 @@ private async Task SetDynamicInstrumentationStateAsync(TracerSettings tracerSett
391418
return;
392419
}
393420

394-
var requestedState = debuggerSettings.DynamicInstrumentationEnabled;
421+
var requestedState = DebuggerSettings.DynamicInstrumentationEnabled;
395422

396423
if (!requestedState)
397424
{
398-
DisableDynamicInstrumentation();
425+
DisableDynamicInstrumentation(DebuggerSettings.DynamicSettings.DynamicInstrumentationEnabled == false);
399426
return;
400427
}
401428

402-
lock (_syncLock)
429+
if (Volatile.Read(ref _diState) != 0)
403430
{
404-
if (DynamicInstrumentation != null)
405-
{
406-
Log.Debug("Dynamic Instrumentation is already initialized");
407-
return;
408-
}
431+
Log.Debug("Dynamic Instrumentation is already initialized");
432+
return;
409433
}
410434

411435
var tracerManager = TracerManager.Instance;
@@ -425,7 +449,7 @@ private async Task SetDynamicInstrumentationStateAsync(TracerSettings tracerSett
425449
return;
426450
}
427451

428-
EnableDynamicInstrumentation(discoveryService, tracerManager);
452+
EnableDynamicInstrumentation(discoveryService, tracerManager, tracerSettings, cancellationToken);
429453
}
430454
catch (Exception ex)
431455
{
@@ -443,60 +467,107 @@ private async Task SetDynamicInstrumentationStateAsync(TracerSettings tracerSett
443467

444468
Log.Error(ex, "Error initializing Dynamic Instrumentation");
445469
}
470+
}
446471

447-
void DisableDynamicInstrumentation()
472+
private void EnableDynamicInstrumentation(IDiscoveryService discoveryService, TracerManager tracerManager, TracerSettings tracerSettings, CancellationToken cancellationToken)
473+
{
474+
if (Interlocked.CompareExchange(ref _diState, 1, 0) != 0)
448475
{
449-
Log.Debug("Disabling Dynamic Instrumentation");
476+
return; // someone else is initializing or done
477+
}
478+
479+
DynamicInstrumentation? di = null;
480+
481+
try
482+
{
483+
di = DebuggerFactory.CreateDynamicInstrumentation(
484+
discoveryService,
485+
RcmSubscriptionManager.Instance,
486+
tracerSettings,
487+
ServiceName,
488+
DebuggerSettings,
489+
tracerManager.GitMetadataTagsProvider);
490+
491+
if (cancellationToken.IsCancellationRequested || _processExit.Task.IsCompleted)
492+
{
493+
Interlocked.Exchange(ref _diState, 0);
494+
SafeDisposal.TryDispose(di);
495+
return;
496+
}
450497

451-
var disabledViaRemoteConfig = debuggerSettings.DynamicSettings.DynamicInstrumentationEnabled == false;
498+
di.Initialize();
452499

453500
lock (_syncLock)
454501
{
455-
if (DynamicInstrumentation != null)
502+
var enabled = DebuggerSettings.DynamicInstrumentationEnabled;
503+
var initialized = _dynamicInstrumentation is { IsDisposed: false };
504+
var state = _diState;
505+
506+
if (_processExit.Task.IsCompleted ||
507+
!enabled ||
508+
cancellationToken.IsCancellationRequested ||
509+
state != 1 ||
510+
initialized)
456511
{
457-
SafeDisposal.TryDispose(_dynamicInstrumentation);
458-
_dynamicInstrumentation = null;
459-
TracerManager.Instance.Telemetry.ProductChanged(TelemetryProductType.DynamicInstrumentation, enabled: false, error: null);
512+
if (state == 1)
513+
{
514+
_diState = 0;
515+
}
516+
}
517+
else
518+
{
519+
_dynamicInstrumentation = di;
520+
di = null;
521+
_diState = 2; // initialized
460522
}
461523
}
462524

463-
if (disabledViaRemoteConfig)
464-
{
465-
Log.Debug("Dynamic Instrumentation is disabled by remote enablement. To enable it, re-enable it via Datadog UI");
466-
}
467-
else
525+
if (di != null)
468526
{
469-
Log.Debug("Dynamic Instrumentation is disabled. To enable it, please set {DynamicInstrumentationEnabled} environment variable to 'true'.", ConfigurationKeys.Debugger.DynamicInstrumentationEnabled);
527+
SafeDisposal.TryDispose(di);
528+
return;
470529
}
471-
}
472530

473-
void EnableDynamicInstrumentation(IDiscoveryService discoveryService, TracerManager tracerManager)
531+
tracerManager.Telemetry.ProductChanged(TelemetryProductType.DynamicInstrumentation, enabled: true, error: null);
532+
Log.Debug("Dynamic Instrumentation has been created");
533+
}
534+
catch (Exception ex)
474535
{
475-
DynamicInstrumentation? instance = null;
476-
var created = false;
477-
lock (_syncLock)
478-
{
479-
if (DynamicInstrumentation == null)
480-
{
481-
_dynamicInstrumentation = DebuggerFactory.CreateDynamicInstrumentation(
482-
discoveryService,
483-
RcmSubscriptionManager.Instance,
484-
tracerSettings,
485-
ServiceName,
486-
debuggerSettings,
487-
tracerManager.GitMetadataTagsProvider);
488-
created = true;
489-
}
536+
Log.Error(ex, "Fail to initialize Dynamic Instrumentation");
537+
Interlocked.CompareExchange(ref _diState, 0, 1);
538+
SafeDisposal.TryDispose(di);
539+
}
540+
}
490541

491-
instance = _dynamicInstrumentation;
492-
}
542+
private void DisableDynamicInstrumentation(bool dynamicallyDisabled)
543+
{
544+
Log.Debug("Disabling Dynamic Instrumentation");
493545

494-
if (created && instance != null)
546+
bool disabled = false;
547+
lock (_syncLock)
548+
{
549+
if (_dynamicInstrumentation != null)
495550
{
496-
Log.Debug("Dynamic Instrumentation has been created");
497-
instance.Initialize();
498-
tracerManager.Telemetry.ProductChanged(TelemetryProductType.DynamicInstrumentation, enabled: true, error: null);
551+
SafeDisposal.TryDispose(_dynamicInstrumentation);
552+
_dynamicInstrumentation = null;
553+
disabled = true;
499554
}
555+
556+
_diState = 0;
557+
}
558+
559+
if (disabled)
560+
{
561+
TracerManager.Instance.Telemetry.ProductChanged(TelemetryProductType.DynamicInstrumentation, enabled: false, error: null);
562+
}
563+
564+
if (dynamicallyDisabled)
565+
{
566+
Log.Debug("Dynamic Instrumentation is disabled by remote enablement. To enable it, re-enable it via Datadog UI");
567+
}
568+
else
569+
{
570+
Log.Debug("Dynamic Instrumentation is disabled. To enable it, please set {DynamicInstrumentationEnabled} environment variable to 'true'.", ConfigurationKeys.Debugger.DynamicInstrumentationEnabled);
500571
}
501572
}
502573

@@ -536,6 +607,7 @@ private void ShutdownTasks(Exception? ex)
536607
}
537608

538609
_processExit.TrySetResult(true);
610+
Volatile.Write(ref _diState, 0);
539611

540612
if (ex != null)
541613
{

0 commit comments

Comments
 (0)