From 45c234f1765114582ecc1e912e84947ce31dfc56 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 11:09:43 -0700 Subject: [PATCH 01/21] Add probe callbacks --- .../FunctionProbes/FunctionProbesManager.cs | 174 +++++++++++++++--- .../FunctionProbes/IFunctionProbesManager.cs | 9 +- .../FunctionProbes/LogEmittingProbes.cs | 2 - .../ParameterCapturingStrings.Designer.cs | 9 + .../ParameterCapturingStrings.resx | 3 + .../Pipeline/ParameterCapturingPipeline.cs | 17 +- .../MutatingMonitorProfiler.def | 4 +- .../ProbeInstrumentation.cpp | 161 +++++++++++++++- .../ProbeInstrumentation.h | 22 +++ .../ParameterCapturingPipelineTests.cs | 59 +++++- .../FunctionProbes/FunctionProbesScenario.cs | 125 +++++-------- 11 files changed, 461 insertions(+), 124 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index b9507d3c1af..06df4b31be7 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -6,8 +6,11 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Globalization; using System.Reflection; using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.FunctionProbes { @@ -25,50 +28,160 @@ private static extern void RequestFunctionProbeInstallation( uint count, [MarshalAs(UnmanagedType.LPArray)] uint[] boxingTokens, [MarshalAs(UnmanagedType.LPArray)] uint[] boxingTokenCounts); - - private readonly object _requestLocker = new(); + + private delegate void FunctionProbeRegistrationCallback(int hresult); + private delegate void FunctionProbeInstallationCallback(int hresult); + private delegate void FunctionProbeUninstallationCallback(int hresult); + private delegate void FunctionProbeFaultCallback(ulong uniquifier); + + [DllImport(ProfilerIdentifiers.MutatingProfiler.LibraryRootFileName, CallingConvention = CallingConvention.StdCall, PreserveSig = false)] + private static extern void RegisterFunctionProbeCallbacks( + FunctionProbeRegistrationCallback onRegistration, + FunctionProbeInstallationCallback onInstallation, + FunctionProbeUninstallationCallback onUninstallation, + FunctionProbeFaultCallback onFault); + + [DllImport(ProfilerIdentifiers.MutatingProfiler.LibraryRootFileName, CallingConvention = CallingConvention.StdCall, PreserveSig = false)] + private static extern void UnregisterFunctionProbeCallbacks(); + + private long _probeState; + private const long ProbeStateUninitialized = default(long); + private const long ProbeStateUninstalled = 1; + private const long ProbeStateUninstalling = 2; + private const long ProbeStateInstalling = 3; + private const long ProbeStateInstalled = 4; + private const long ProbeStateUnrecoverable = 5; + + private readonly TaskCompletionSource _probeRegistrationTaskSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + private TaskCompletionSource? _installationTaskSource; + private TaskCompletionSource? _uninstallationTaskSource; + private long _disposedState; - private bool _isCapturing; + + public event EventHandler? OnProbeFault; public FunctionProbesManager(IFunctionProbes probes) { ProfilerResolver.InitializeResolver(); + RegisterFunctionProbeCallbacks(OnRegistration, OnInstallation, OnUninstallation, OnFault); RequestFunctionProbeRegistration(FunctionProbesStub.GetProbeFunctionId()); + FunctionProbesStub.Instance = probes; } - public void StopCapturing() + private void OnRegistration(int hresult) { - lock (_requestLocker) + TransitionStateFromHr(_probeRegistrationTaskSource, hresult, + expectedState: ProbeStateUninitialized, + succeededState: ProbeStateUninstalled, + failedState: ProbeStateUnrecoverable); + } + + private void OnInstallation(int hresult) + { + TransitionStateFromHr(_installationTaskSource, hresult, + expectedState: ProbeStateInstalling, + succeededState: ProbeStateInstalled, + failedState: ProbeStateUninstalled); + } + + private void OnUninstallation(int hresult) + { + TransitionStateFromHr(_uninstallationTaskSource, hresult, + expectedState: ProbeStateUninstalling, + succeededState: ProbeStateUninstalled, + failedState: ProbeStateInstalled); + } + + private void OnFault(ulong uniquifier) + { + OnProbeFault?.Invoke(this, uniquifier); + } + + private void TransitionStateFromHr(TaskCompletionSource? taskCompletionSource, int hresult, long expectedState, long succeededState, long failedState) + { + Exception? ex = Marshal.GetExceptionForHR(hresult); + long newState = (ex == null) ? succeededState : failedState; + + if (expectedState != Interlocked.CompareExchange(ref _probeState, newState, expectedState)) { - if (!_isCapturing) - { - return; - } + // Unexpected, the profiler is in a different state than us. + StateMismatch(expectedState); + return; + } - FunctionProbesStub.InstrumentedMethodCache = null; - RequestFunctionProbeUninstallation(); + if (ex == null) + { + _ = taskCompletionSource?.TrySetResult(); + } + else + { + _ = taskCompletionSource?.TrySetException(ex); + } + } + + private void StateMismatch(long expected) + { + InvalidOperationException ex = new(string.Format(CultureInfo.InvariantCulture, ParameterCapturingStrings.ErrorMessage_ProbeStateMismatchFormatString, expected, _probeState)); + + _probeState = ProbeStateUnrecoverable; + _ = _installationTaskSource?.TrySetException(ex); + _ = _uninstallationTaskSource?.TrySetException(ex); + _ = _probeRegistrationTaskSource?.TrySetException(ex); + } + + public async Task StopCapturingAsync(CancellationToken token) + { + if (ProbeStateInstalled != Interlocked.CompareExchange(ref _probeState, ProbeStateUninstalling, ProbeStateInstalled)) + { + throw new InvalidOperationException(); + } - _isCapturing = false; + _uninstallationTaskSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + try + { + StopCapturingCore(); + } + catch + { + _uninstallationTaskSource = null; + _probeState = ProbeStateInstalled; + throw; } + + await _uninstallationTaskSource.Task.WaitAsync(token).ConfigureAwait(false); } - public void StartCapturing(IList methods) + private void StopCapturingCore() + { + if (_probeState != ProbeStateUninstalled) + { + return; + } + + FunctionProbesStub.InstrumentedMethodCache = null; + RequestFunctionProbeUninstallation(); + } + + + public async Task StartCapturingAsync(IList methods, CancellationToken token) { if (methods.Count == 0) { throw new ArgumentException(nameof(methods)); } - Dictionary newMethodCache = new(methods.Count); - lock (_requestLocker) + await _probeRegistrationTaskSource.Task.WaitAsync(token).ConfigureAwait(false); + + if (ProbeStateUninstalled != Interlocked.CompareExchange(ref _probeState, ProbeStateInstalling, ProbeStateUninstalled)) { - if (_isCapturing) - { - throw new InvalidOperationException(); - } + throw new InvalidOperationException(); + } + try + { + Dictionary newMethodCache = new(methods.Count); List functionIds = new(methods.Count); List argumentCounts = new(methods.Count); List boxingTokens = new(); @@ -78,7 +191,7 @@ public void StartCapturing(IList methods) ulong functionId = method.GetFunctionId(); if (functionId == 0) { - return; + throw new NotSupportedException(method.Name); } uint[] methodBoxingTokens = BoxingTokens.GetBoxingTokens(method); @@ -94,14 +207,23 @@ public void StartCapturing(IList methods) } FunctionProbesStub.InstrumentedMethodCache = new ReadOnlyDictionary(newMethodCache); + + _installationTaskSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); RequestFunctionProbeInstallation( functionIds.ToArray(), (uint)functionIds.Count, boxingTokens.ToArray(), argumentCounts.ToArray()); - - _isCapturing = true; } + catch + { + FunctionProbesStub.InstrumentedMethodCache = null; + _probeState = ProbeStateUninstalled; + _installationTaskSource = null; + throw; + } + + await _installationTaskSource.Task.WaitAsync(token).ConfigureAwait(false); } public void Dispose() @@ -110,13 +232,17 @@ public void Dispose() return; FunctionProbesStub.Instance = null; + + _ = _installationTaskSource?.TrySetCanceled(); + _ = _uninstallationTaskSource?.TrySetCanceled(); + try { - StopCapturing(); + UnregisterFunctionProbeCallbacks(); + StopCapturingCore(); } catch { - } } } diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs index 7a98d4f6f4a..828e29398c0 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs @@ -4,13 +4,18 @@ using System; using System.Collections.Generic; using System.Reflection; +using System.Threading; +using System.Threading.Tasks; namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.FunctionProbes { internal interface IFunctionProbesManager : IDisposable { - public void StartCapturing(IList methods); + public Task StartCapturingAsync(IList methods, CancellationToken token); - public void StopCapturing(); + public Task StopCapturingAsync(CancellationToken token); + + + public event EventHandler OnProbeFault; } } diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs index 5427e7610e2..bb4836c464d 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs @@ -1,8 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.Extensions.Logging; - namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.FunctionProbes { internal sealed class LogEmittingProbes : IFunctionProbes diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs index e1c0b8a6bba..89bdbc39ad2 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs @@ -61,6 +61,15 @@ internal ParameterCapturingStrings() { } } + /// + /// Looks up a localized string similar to Mismatch probe state. Expected '{0}', was '{1}'.. + /// + internal static string ErrorMessage_ProbeStateMismatchFormatString { + get { + return ResourceManager.GetString("ErrorMessage_ProbeStateMismatchFormatString", resourceCulture); + } + } + /// /// Looks up a localized string similar to Unable to create an ILogger instance in the target process.. /// diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx index d944e27283f..bc9ac4961b9 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx @@ -117,6 +117,9 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + Mismatch probe state. Expected '{0}', was '{1}'. + Unable to create an ILogger instance in the target process. diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs index 50dd39e415c..dbc6673f07c 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs @@ -50,8 +50,16 @@ public async Task RunAsync(CancellationToken stoppingToken) while (!stoppingToken.IsCancellationRequested) { CapturingRequest request = await _requestQueue.Reader.ReadAsync(stoppingToken); - if (!TryStartCapturing(request.Payload)) + + void onFault(object? sender, ulong uniquifier) + { + _ = request.StopRequest.TrySetResult(); + } + _probeManager.OnProbeFault += onFault; + + if (!await TryStartCapturingAsync(request.Payload, stoppingToken).ConfigureAwait(false)) { + _probeManager.OnProbeFault -= onFault; continue; } @@ -67,7 +75,8 @@ public async Task RunAsync(CancellationToken stoppingToken) } - _probeManager.StopCapturing(); + _probeManager.OnProbeFault -= onFault; + await _probeManager.StopCapturingAsync(stoppingToken).ConfigureAwait(false); _callbacks.CapturingStop(request.Payload.RequestId); _ = _allRequests.TryRemove(request.Payload.RequestId, out _); @@ -76,7 +85,7 @@ public async Task RunAsync(CancellationToken stoppingToken) // Private method for work that happens inside the pipeline's RunAsync // so use callbacks instead of throwing exceptions. - private bool TryStartCapturing(StartCapturingParametersPayload request) + private async Task TryStartCapturingAsync(StartCapturingParametersPayload request, CancellationToken token) { try { @@ -103,7 +112,7 @@ private bool TryStartCapturing(StartCapturingParametersPayload request) throw ex; } - _probeManager.StartCapturing(methods); + await _probeManager.StartCapturingAsync(methods, token).ConfigureAwait(false); _callbacks.CapturingStart(request, methods); return true; diff --git a/src/Profilers/MutatingMonitorProfiler/MutatingMonitorProfiler.def b/src/Profilers/MutatingMonitorProfiler/MutatingMonitorProfiler.def index 5e7d2983be2..9b48fb13732 100644 --- a/src/Profilers/MutatingMonitorProfiler/MutatingMonitorProfiler.def +++ b/src/Profilers/MutatingMonitorProfiler/MutatingMonitorProfiler.def @@ -6,4 +6,6 @@ EXPORTS DllMain PRIVATE RequestFunctionProbeRegistration PRIVATE RequestFunctionProbeInstallation PRIVATE - RequestFunctionProbeUninstallation PRIVATE \ No newline at end of file + RequestFunctionProbeUninstallation PRIVATE + RegisterFunctionProbeCallbacks PRIVATE + UnregisterFunctionProbeCallbacks PRIVATE \ No newline at end of file diff --git a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp index 716d4148124..03959e7ceb8 100644 --- a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp +++ b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp @@ -14,6 +14,22 @@ using namespace std; #define DLLEXPORT #endif +typedef void (STDMETHODCALLTYPE *ProbeRegistrationCallback)(HRESULT); +typedef void (STDMETHODCALLTYPE *ProbeInstallationCallback)(HRESULT); +typedef void (STDMETHODCALLTYPE *ProbeUninstallationCallback)(HRESULT); +typedef void (STDMETHODCALLTYPE *ProbeFaultCallback)(ULONG64); + +typedef struct _PROBE_MANAGEMENT_CALLBACKS +{ + ProbeRegistrationCallback pProbeRegistrationCallback; + ProbeInstallationCallback pProbeInstallationCallback; + ProbeUninstallationCallback pProbeUninstallationCallback; + ProbeFaultCallback pProbeFaultCallback; +} PROBE_MANAGEMENT_CALLBACKS; + +mutex g_probeManagementCallbacksMutex; // guards g_probeManagementCallbacks +PROBE_MANAGEMENT_CALLBACKS g_probeManagementCallbacks = {}; + BlockingQueue g_probeManagementQueue; ProbeInstrumentation::ProbeInstrumentation(const shared_ptr& logger, ICorProfilerInfo12* profilerInfo) : @@ -47,9 +63,87 @@ HRESULT ProbeInstrumentation::RegisterFunctionProbe(FunctionID enterProbeId) HRESULT ProbeInstrumentation::InitBackgroundService() { m_probeManagementThread = thread(&ProbeInstrumentation::WorkerThread, this); + // + // Create a dedicated thread for managed callbacks. + // Performing the callbacks will taint the calling thread, + // preventing it from using certain ICorProfiler APIs marked as "unsafe". + // + // The unsafe ICorProfiler APIs will believe our thread is calling them asynchronously, + // failing the request with CORPROF_E_UNSUPPORTED_CALL_SEQUENCE, + // even if our thread has already been initialized with ICorProfiler. + // + m_managedCallbackThread = thread(&ProbeInstrumentation::ManagedCallbackThread, this); return S_OK; } +void ProbeInstrumentation::ManagedCallbackThread() +{ + HRESULT hr = m_pCorProfilerInfo->InitializeCurrentThread(); + if (FAILED(hr)) + { + m_pLogger->Log(LogLevel::Error, _LS("Unable to initialize thread: 0x%08x"), hr); + return; + } + + while (true) + { + MANAGED_CALLBACK_REQUEST request; + hr = m_managedCallbackQueue.BlockingDequeue(request); + if (hr != S_OK) + { + break; + } + + switch (request.instruction) + { + case ProbeWorkerInstruction::REGISTER_PROBE: + { + lock_guard lock(g_probeManagementCallbacksMutex); + if (g_probeManagementCallbacks.pProbeRegistrationCallback != nullptr) + { + g_probeManagementCallbacks.pProbeRegistrationCallback(request.payload.hr); + } + } + break; + + case ProbeWorkerInstruction::INSTALL_PROBES: + { + lock_guard lock(g_probeManagementCallbacksMutex); + if (g_probeManagementCallbacks.pProbeInstallationCallback != nullptr) + { + g_probeManagementCallbacks.pProbeInstallationCallback(request.payload.hr); + } + } + break; + + case ProbeWorkerInstruction::FAULTING_PROBE: + { + lock_guard lock(g_probeManagementCallbacksMutex); + if (g_probeManagementCallbacks.pProbeFaultCallback != nullptr) + { + g_probeManagementCallbacks.pProbeFaultCallback(static_cast(request.payload.functionId)); + } + } + break; + + case ProbeWorkerInstruction::UNINSTALL_PROBES: + { + lock_guard lock(g_probeManagementCallbacksMutex); + if (g_probeManagementCallbacks.pProbeUninstallationCallback != nullptr) + { + g_probeManagementCallbacks.pProbeUninstallationCallback(request.payload.hr); + } + } + break; + + default: + m_pLogger->Log(LogLevel::Error, _LS("Unknown message")); + break; + } + } +} + + void ProbeInstrumentation::WorkerThread() { HRESULT hr = m_pCorProfilerInfo->InitializeCurrentThread(); @@ -59,6 +153,7 @@ void ProbeInstrumentation::WorkerThread() return; } + MANAGED_CALLBACK_REQUEST callbackRequest = {}; while (true) { PROBE_WORKER_PAYLOAD payload; @@ -68,6 +163,7 @@ void ProbeInstrumentation::WorkerThread() break; } + callbackRequest.instruction = payload.instruction; switch (payload.instruction) { case ProbeWorkerInstruction::REGISTER_PROBE: @@ -76,6 +172,8 @@ void ProbeInstrumentation::WorkerThread() { m_pLogger->Log(LogLevel::Error, _LS("Failed to register function probe: 0x%08x"), hr); } + callbackRequest.payload.hr = hr; + m_managedCallbackQueue.Enqueue(callbackRequest); break; case ProbeWorkerInstruction::INSTALL_PROBES: @@ -84,11 +182,15 @@ void ProbeInstrumentation::WorkerThread() { m_pLogger->Log(LogLevel::Error, _LS("Failed to install probes: 0x%08x"), hr); } + callbackRequest.payload.hr = hr; + m_managedCallbackQueue.Enqueue(callbackRequest); break; case ProbeWorkerInstruction::FAULTING_PROBE: m_pLogger->Log(LogLevel::Error, _LS("Function probe faulting in function: 0x%08x"), payload.functionId); - __fallthrough; + callbackRequest.payload.functionId = payload.functionId; + m_managedCallbackQueue.Enqueue(callbackRequest); + break; case ProbeWorkerInstruction::UNINSTALL_PROBES: hr = UninstallProbes(); @@ -96,6 +198,8 @@ void ProbeInstrumentation::WorkerThread() { m_pLogger->Log(LogLevel::Error, _LS("Failed to uninstall probes: 0x%08x"), hr); } + callbackRequest.payload.hr = hr; + m_managedCallbackQueue.Enqueue(callbackRequest); break; default: @@ -113,15 +217,13 @@ void ProbeInstrumentation::DisableIncomingRequests() void ProbeInstrumentation::ShutdownBackgroundService() { DisableIncomingRequests(); + m_managedCallbackQueue.Complete(); + m_probeManagementThread.join(); m_probeManagementThread.join(); } void STDMETHODCALLTYPE ProbeInstrumentation::OnFunctionProbeFault(ULONG64 uniquifier) { - // - // Faulting behavior: When any function probe faults, uninstall all probes. - // - PROBE_WORKER_PAYLOAD payload = {}; payload.instruction = ProbeWorkerInstruction::FAULTING_PROBE; @@ -252,7 +354,7 @@ HRESULT ProbeInstrumentation::InstallProbes(vector lock(g_probeManagementCallbacksMutex); + + // Just check one of the callbacks to see if they're already set, + // a mixture of set and unset callbacks is not supported. + if (g_probeManagementCallbacks.pProbeRegistrationCallback != nullptr) + { + return E_FAIL; + } + + g_probeManagementCallbacks.pProbeRegistrationCallback = pRegistrationCallback; + g_probeManagementCallbacks.pProbeInstallationCallback = pInstallationCallback; + g_probeManagementCallbacks.pProbeUninstallationCallback = pUninstallationCallback; + g_probeManagementCallbacks.pProbeFaultCallback = pFaultCallback; + + return S_OK; +} + +STDAPI DLLEXPORT UnregisterFunctionProbeCallbacks() +{ + lock_guard lock(g_probeManagementCallbacksMutex); + g_probeManagementCallbacks = {}; return S_OK; } \ No newline at end of file diff --git a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h index 15b93ce110a..6e57ec196fa 100644 --- a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h +++ b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h @@ -11,6 +11,7 @@ #include "CallbackDefinitions.h" #include "Logging/Logger.h" #include "CommonUtilities/PairHash.h" +#include "CommonUtilities/BlockingQueue.h" #include #include @@ -41,6 +42,23 @@ typedef struct _PROBE_WORKER_PAYLOAD std::vector requests; } PROBE_WORKER_PAYLOAD; +enum class ManagedCallbackType +{ + REGISTER_PROBE, + INSTALL_PROBES, + UNINSTALL_PROBES, + FAULTING_PROBE +}; + +typedef struct _MANAGED_CALLBACK_REQUEST +{ + ProbeWorkerInstruction instruction; + union { + HRESULT hr; + FunctionID functionId; + } payload; +} MANAGED_CALLBACK_REQUEST; + class ProbeInstrumentation { private: @@ -51,6 +69,9 @@ class ProbeInstrumentation std::unique_ptr m_pAssemblyProbePrep; /* Probe management */ + std::thread m_managedCallbackThread; + BlockingQueue m_managedCallbackQueue; + std::thread m_probeManagementThread; std::unordered_map, INSTRUMENTATION_REQUEST, PairHash> m_activeInstrumentationRequests; std::mutex m_instrumentationProcessingMutex; @@ -58,6 +79,7 @@ class ProbeInstrumentation private: void WorkerThread(); + void ManagedCallbackThread(); HRESULT RegisterFunctionProbe(FunctionID enterProbeId); HRESULT InstallProbes(std::vector& requests); HRESULT UninstallProbes(); diff --git a/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs b/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs index 9e44e69358a..d93dd287f53 100644 --- a/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs +++ b/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing; using Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.FunctionProbes; using Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.Pipeline; using Microsoft.Diagnostics.Monitoring.StartupHook.MonitorMessageDispatcher.Models; @@ -22,20 +23,29 @@ internal sealed class TestFunctionProbesManager : IFunctionProbesManager private readonly Action> _onStart; private readonly Action _onStop; + public event EventHandler OnProbeFault; + public TestFunctionProbesManager(Action> onStart = null, Action onStop = null) { _onStart = onStart; _onStop = onStop; } - public void StartCapturing(IList methods) + public void TriggerFault(ulong uniquifier) + { + OnProbeFault?.Invoke(this, uniquifier); + } + + public Task StartCapturingAsync(IList methods, CancellationToken token) { _onStart?.Invoke(methods); + return Task.CompletedTask; } - public void StopCapturing() + public Task StopCapturingAsync(CancellationToken token) { _onStop?.Invoke(); + return Task.CompletedTask; } public void Dispose() @@ -264,6 +274,51 @@ public async Task Request_StopsAfterDuration() Assert.Equal(payload.RequestId, stoppedRequest); } + [Fact] + public async Task Request_StopsAndNotifiesAfterFault() + { + // Arrange + using CancellationTokenSource cts = new(); + cts.CancelAfter(CommonTestTimeouts.GeneralTimeout); + + TaskCompletionSource onStopCallbackSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + TaskCompletionSource<(Guid, IList)> onStartCallbackSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + + using IDisposable registration = cts.Token.Register(() => + { + _ = onStartCallbackSource.TrySetCanceled(cts.Token); + _ = onStopCallbackSource.TrySetCanceled(cts.Token); + }); + + TestFunctionProbesManager probeManager = new(); + + TestParameterCapturingCallbacks callbacks = new( + onCapturingStart: (payload, methods) => + { + onStartCallbackSource.TrySetResult((payload.RequestId, methods)); + }, + onCapturingStop: (requestId) => + { + onStopCallbackSource.TrySetResult(requestId); + }); + + ParameterCapturingPipeline pipeline = new(probeManager, callbacks); + StartCapturingParametersPayload payload = CreateStartCapturingPayload(Timeout.InfiniteTimeSpan); + + Task pipelineTask = pipeline.RunAsync(cts.Token); + pipeline.SubmitRequest(payload); + (Guid startedRequest, IList methods) = await onStartCallbackSource.Task; + Assert.Equal(payload.RequestId, startedRequest); + MethodInfo instrumentedMethod = Assert.Single(methods); + + // Act + probeManager.TriggerFault(instrumentedMethod.GetFunctionId()); + + // Assert + Guid stoppedRequest = await onStopCallbackSource.Task; + Assert.Equal(payload.RequestId, stoppedRequest); + } + [Fact] public async Task RunAsync_ThrowsOnCapturingStopFailure() { diff --git a/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs b/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs index 58ffa0a9e41..bd152b665df 100644 --- a/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs +++ b/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing; using Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.FunctionProbes; using Microsoft.Diagnostics.Monitoring.TestCommon; using SampleMethods; @@ -79,20 +80,33 @@ public static CliCommand Command() private static async Task Test_ProbeInstallationAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token) { - await WaitForProbeInstallationAsync(probeManager, probeProxy, Array.Empty(), token); + MethodInfo method = typeof(StaticTestMethodSignatures).GetMethod(nameof(StaticTestMethodSignatures.NoArgs)); + probeProxy.RegisterPerFunctionProbe(method, (object[] args) => { }); + + await probeManager.StartCapturingAsync(new[] { method }, token); + StaticTestMethodSignatures.NoArgs(); + + Assert.Equal(1, probeProxy.GetProbeInvokeCount(method)); } private static async Task Test_ProbeUninstallationAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token) { - await WaitForProbeInstallationAsync(probeManager, probeProxy, Array.Empty(), token); - await WaitForProbeUninstallationAsync(probeManager, probeProxy, token); + MethodInfo method = typeof(StaticTestMethodSignatures).GetMethod(nameof(StaticTestMethodSignatures.NoArgs)); + probeProxy.RegisterPerFunctionProbe(method, (object[] args) => { }); + + await probeManager.StartCapturingAsync(new[] { method }, token); + StaticTestMethodSignatures.NoArgs(); + + await probeManager.StopCapturingAsync(token); + StaticTestMethodSignatures.NoArgs(); + + Assert.Equal(1, probeProxy.GetProbeInvokeCount(method)); } private static async Task Test_ProbeReinstallationAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token) { - await WaitForProbeInstallationAsync(probeManager, probeProxy, Array.Empty(), token); - await WaitForProbeUninstallationAsync(probeManager, probeProxy, token); - await WaitForProbeInstallationAsync(probeManager, probeProxy, Array.Empty(), token); + await Test_ProbeUninstallationAsync(probeManager, probeProxy, token); + await Test_ProbeUninstallationAsync(probeManager, probeProxy, token); } private static async Task Test_CapturePrimitivesAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token) @@ -172,7 +186,7 @@ private static async Task Test_GenericMethodsAsync(FunctionProbesManager probeMa probeProxy.RegisterPerFunctionProbe(method, (object[] actualArgs) => { }); - await WaitForProbeInstallationAsync(probeManager, probeProxy, new[] { method }, token); + await probeManager.StartCapturingAsync(new[] { method }, token); new GenericTestMethodSignatures().GenericParameters(false, 10, "hello world"); new GenericTestMethodSignatures().GenericParameters("", new object(), 10); @@ -218,14 +232,27 @@ private static async Task Test_ExceptionThrownByProbeAsync(FunctionProbesManager throw new InvalidOperationException(); }); - await WaitForProbeInstallationAsync(probeManager, probeProxy, new[] { method }, token); + await probeManager.StartCapturingAsync(new[] { method }, token); - StaticTestMethodSignatures.ExceptionRegionAtBeginningOfMethod(null); + TaskCompletionSource faultingUniquifierSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + void onFault(object caller, ulong uniquifier) + { + _ = faultingUniquifierSource.TrySetResult(uniquifier); + } + probeManager.OnProbeFault += onFault; - Assert.Equal(1, probeProxy.GetProbeInvokeCount(method)); + StaticTestMethodSignatures.ExceptionRegionAtBeginningOfMethod(null); - // Probes should be uninstalled now. - await WaitForProbeUninstallationAsync(probeManager, probeProxy, token, explicitStopCaptureCall: false); + // Faults are handled asynchronously, so wait for the event to propagate + try + { + ulong faultingUniquifier = await faultingUniquifierSource.Task.WaitAsync(token); + Assert.Equal(method.GetFunctionId(), faultingUniquifier); + } + finally + { + probeManager.OnProbeFault -= onFault; + } } private static async Task Test_RecursingProbeAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token) @@ -236,7 +263,7 @@ private static async Task Test_RecursingProbeAsync(FunctionProbesManager probeMa StaticTestMethodSignatures.NoArgs(); }); - await WaitForProbeInstallationAsync(probeManager, probeProxy, new[] { method }, token); + await probeManager.StartCapturingAsync(new[] { method }, token); StaticTestMethodSignatures.NoArgs(); @@ -253,8 +280,7 @@ private static async Task Test_RequestInstallationOnProbeFunctionAsync(FunctionP using CancellationTokenSource timeoutSource = CancellationTokenSource.CreateLinkedTokenSource(token); timeoutSource.CancelAfter(TimeSpan.FromSeconds(5)); - // There's currently no notification mechanism for determining probe installation success, wait for timeout instead. - await Assert.ThrowsAnyAsync(async () => await WaitForProbeInstallationAsync(probeManager, probeProxy, new[] { method }, timeoutSource.Token)); + await Assert.ThrowsAsync(async () => await probeManager.StartCapturingAsync(new[] { method }, token)); } private static async Task Test_AssertsInProbesAreCaughtAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token) @@ -322,7 +348,7 @@ private static async Task RunTestCaseWithCustomInvokerAsync(FunctionProbesManage } }); - await WaitForProbeInstallationAsync(probeManager, probeProxy, new[] { method }, token); + await probeManager.StartCapturingAsync(new[] { method }, token); await invoker().WaitAsync(token); @@ -333,72 +359,5 @@ private static async Task RunTestCaseWithCustomInvokerAsync(FunctionProbesManage throw assertFailure; } } - - private static async Task WaitForProbeUninstallationAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, CancellationToken token, bool explicitStopCaptureCall = true) - { - if (explicitStopCaptureCall) - { - probeManager.StopCapturing(); - } - - MethodInfo uninstallationTestMethod = typeof(FunctionProbesScenario).GetMethod(nameof(FunctionProbesScenario.UninstallationTestStub)); - Assert.NotNull(uninstallationTestMethod); - - probeProxy.RegisterPerFunctionProbe(uninstallationTestMethod, (object[] args) => { }); - - while (!token.IsCancellationRequested) - { - int currentCount = probeProxy.GetProbeInvokeCount(uninstallationTestMethod); - uninstallationTestMethod.Invoke(null, null); - if (currentCount == probeProxy.GetProbeInvokeCount(uninstallationTestMethod)) - { - return; - } - - await Task.Delay(100, token); - } - - token.ThrowIfCancellationRequested(); - } - - private static async Task WaitForProbeInstallationAsync(FunctionProbesManager probeManager, PerFunctionProbeProxy probeProxy, IList methods, CancellationToken token) - { - // Register the uninstallation test method as well so WaitForProbeUninstallationAsync can function - MethodInfo uninstallationTestMethod = typeof(FunctionProbesScenario).GetMethod(nameof(FunctionProbesScenario.UninstallationTestStub)); - Assert.NotNull(uninstallationTestMethod); - - MethodInfo installationTestMethod = typeof(FunctionProbesScenario).GetMethod(nameof(FunctionProbesScenario.InstallationTestStub)); - Assert.NotNull(installationTestMethod); - - probeProxy.RegisterPerFunctionProbe(installationTestMethod, (object[] args) => { }); - - List methodsToCapture = new(methods.Count + 2) - { - installationTestMethod, - uninstallationTestMethod - }; - methodsToCapture.AddRange(methods); - probeManager.StartCapturing(methodsToCapture); - - while (!token.IsCancellationRequested) - { - installationTestMethod.Invoke(null, null); - if (probeProxy.GetProbeInvokeCount(installationTestMethod) != 0) - { - return; - } - - await Task.Delay(100, token); - } - - token.ThrowIfCancellationRequested(); - } - - public static void InstallationTestStub() - { - } - public static void UninstallationTestStub() - { - } } } From 436ad000354ed9cef00e446df373bb40ad15c3b5 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 11:14:07 -0700 Subject: [PATCH 02/21] Fix uninstallation --- .../ParameterCapturing/FunctionProbes/FunctionProbesManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 06df4b31be7..6a3457f375e 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -155,7 +155,7 @@ public async Task StopCapturingAsync(CancellationToken token) private void StopCapturingCore() { - if (_probeState != ProbeStateUninstalled) + if (_probeState == ProbeStateUninstalled) { return; } From f5154ec71b90a33dde61a5dbb362644fc2318ba7 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 13:41:09 -0700 Subject: [PATCH 03/21] Rework responsibility for fault handling --- .../FunctionProbes/FunctionProbesManager.cs | 16 +++- .../FunctionProbes/IFunctionProbesManager.cs | 2 +- .../ParameterCapturing/InstrumentedMethod.cs | 3 + .../ParameterCapturingService.cs | 88 ++++++++++--------- .../ParameterCapturingStrings.Designer.cs | 9 ++ .../ParameterCapturingStrings.resx | 3 + .../IParameterCapturingPipelineCallbacks.cs | 5 +- .../Pipeline/ParameterCapturingPipeline.cs | 17 ++-- .../ParameterCapturing/Pipeline/ScopeGuard.cs | 28 ++++++ .../ProbeInstrumentation.h | 3 +- .../ParameterCapturingPipelineTests.cs | 44 +++++++--- .../FunctionProbes/FunctionProbesScenario.cs | 10 +-- 12 files changed, 157 insertions(+), 71 deletions(-) create mode 100644 src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 6a3457f375e..666b4205875 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -58,7 +58,7 @@ private static extern void RegisterFunctionProbeCallbacks( private long _disposedState; - public event EventHandler? OnProbeFault; + public event EventHandler? OnProbeFault; public FunctionProbesManager(IFunctionProbes probes) { @@ -96,7 +96,19 @@ private void OnUninstallation(int hresult) private void OnFault(ulong uniquifier) { - OnProbeFault?.Invoke(this, uniquifier); + var methodCache = FunctionProbesStub.InstrumentedMethodCache; + if (methodCache == null || + !methodCache.TryGetValue(uniquifier, out InstrumentedMethod? instrumentedMethod)) + { + // + // The probe fault occured in a method that is no longer actively instrumented, ignore. + // This can happen when we request uninstallation of function probes and there's still a thread + // actively in one of the instrumented methods and it happens to fault. + // + return; + } + + OnProbeFault?.Invoke(this, instrumentedMethod); } private void TransitionStateFromHr(TaskCompletionSource? taskCompletionSource, int hresult, long expectedState, long succeededState, long failedState) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs index 828e29398c0..c530da79aea 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs @@ -16,6 +16,6 @@ internal interface IFunctionProbesManager : IDisposable public Task StopCapturingAsync(CancellationToken token); - public event EventHandler OnProbeFault; + public event EventHandler OnProbeFault; } } diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/InstrumentedMethod.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/InstrumentedMethod.cs index b4ec935d614..9d71e3e8fe2 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/InstrumentedMethod.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/InstrumentedMethod.cs @@ -19,6 +19,7 @@ public sealed class InstrumentedMethod public InstrumentedMethod(MethodInfo method, uint[] boxingTokens) { + FunctionId = method.GetFunctionId(); SupportedParameters = BoxingTokens.AreParametersSupported(boxingTokens); MethodWithParametersTemplateString = PrettyPrinter.ConstructTemplateStringFromMethod(method, SupportedParameters); foreach (bool isParameterSupported in SupportedParameters) @@ -67,5 +68,7 @@ private static ParameterCaptureMode ComputeCaptureMode(string? typeName) /// The number of format items equals NumberOfSupportedParameters. /// public string MethodWithParametersTemplateString { get; } + + public ulong FunctionId { get; } } } diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs index 47ee6c9b776..4a58ea2a5b1 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs @@ -19,44 +19,8 @@ namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing { - internal sealed class ParameterCapturingService : BackgroundService, IDisposable + internal sealed class ParameterCapturingService : BackgroundService, IParameterCapturingPipelineCallbacks, IDisposable { - private sealed class ParameterCapturingCallbacks : IParameterCapturingPipelineCallbacks - { - private readonly ILogger _logger; - private readonly ParameterCapturingEventSource _eventSource; - - public ParameterCapturingCallbacks(ILogger logger, ParameterCapturingEventSource eventSource) - { - _logger = logger; - _eventSource = eventSource; - } - - public void CapturingStart(StartCapturingParametersPayload request, IList methods) - { - _eventSource.CapturingStart(request.RequestId); - _logger?.LogInformation( - ParameterCapturingStrings.StartParameterCapturingFormatString, - request.Duration, - methods.Count); - } - - public void CapturingStop(Guid RequestId) - { - _eventSource.CapturingStop(RequestId); - _logger?.LogInformation(ParameterCapturingStrings.StopParameterCapturing); - } - - public void FailedToCapture(Guid RequestId, ParameterCapturingEvents.CapturingFailedReason Reason, string Details) - { - _eventSource.FailedToCapture(RequestId, Reason, Details); - if (Reason == ParameterCapturingEvents.CapturingFailedReason.UnresolvedMethods) - { - _logger?.LogWarning(Details); - } - } - } - private long _disposedState; private ParameterCapturingEvents.ServiceState _serviceState; @@ -65,6 +29,8 @@ public void FailedToCapture(Guid RequestId, ParameterCapturingEvents.CapturingFa private readonly ParameterCapturingEventSource _eventSource = new(); private readonly ParameterCapturingPipeline? _pipeline; + private readonly ILogger? _logger; + public ParameterCapturingService(IServiceProvider services) { try @@ -81,17 +47,16 @@ public ParameterCapturingService(IServiceProvider services) IpcCommand.StopCapturingParameters, OnStopMessage); - ILogger logger = services.GetService>() + _logger = services.GetService>() ?? throw new NotSupportedException(ParameterCapturingStrings.FeatureUnsupported_NoLogger); ILogger systemLogger = services.GetService>() ?? throw new NotSupportedException(ParameterCapturingStrings.FeatureUnsupported_NoLogger); - ParameterCapturingLogger parameterCapturingLogger = new(logger, systemLogger); + ParameterCapturingLogger parameterCapturingLogger = new(_logger, systemLogger); FunctionProbesManager probeManager = new(new LogEmittingProbes(parameterCapturingLogger)); - ParameterCapturingCallbacks callbacks = new(logger, _eventSource); - _pipeline = new ParameterCapturingPipeline(probeManager, callbacks); + _pipeline = new ParameterCapturingPipeline(probeManager, this); } catch (NotSupportedException ex) { @@ -103,6 +68,47 @@ public ParameterCapturingService(IServiceProvider services) } } + #region IParameterCapturingPipelineCallbacks + public void CapturingStart(StartCapturingParametersPayload request, IList methods) + { + _eventSource.CapturingStart(request.RequestId); + _logger?.LogInformation( + ParameterCapturingStrings.StartParameterCapturingFormatString, + request.Duration, + methods.Count); + } + + public void CapturingStop(Guid requestId) + { + _eventSource.CapturingStop(requestId); + _logger?.LogInformation(ParameterCapturingStrings.StopParameterCapturing); + } + + public void FailedToCapture(Guid requestId, ParameterCapturingEvents.CapturingFailedReason reason, string details) + { + _eventSource.FailedToCapture(requestId, reason, details); + if (reason == ParameterCapturingEvents.CapturingFailedReason.UnresolvedMethods) + { + _logger?.LogWarning(details); + } + } + + public void ProbeFault(Guid requestId, InstrumentedMethod faultingMethod) + { + // TODO: Report back this fault on ParameterCapturingEventSource. + _logger?.LogInformation(ParameterCapturingStrings.StoppingParameterCapturingDueToProbeFault, faultingMethod.MethodWithParametersTemplateString); + + try + { + _pipeline?.RequestStop(requestId); + } + catch + { + + } + } + #endregion + private bool IsAvailable() { return _serviceState == ParameterCapturingEvents.ServiceState.Running; diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs index 89bdbc39ad2..e39d101e3ad 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs @@ -133,6 +133,15 @@ internal static string StopParameterCapturing { } } + /// + /// Looks up a localized string similar to Parameter capturing encountered an internal exception when processing '{method}', stopping.. + /// + internal static string StoppingParameterCapturingDueToProbeFault { + get { + return ResourceManager.GetString("StoppingParameterCapturingDueToProbeFault", resourceCulture); + } + } + /// /// Looks up a localized string similar to this. /// diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx index bc9ac4961b9..77e7ce5c265 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx @@ -141,6 +141,9 @@ Stopped parameter capturing. + + Parameter capturing encountered an internal exception when processing '{method}', stopping. + this diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/IParameterCapturingPipelineCallbacks.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/IParameterCapturingPipelineCallbacks.cs index 86b45a9ef11..5c90d6bd3a6 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/IParameterCapturingPipelineCallbacks.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/IParameterCapturingPipelineCallbacks.cs @@ -12,7 +12,8 @@ namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.Fun internal interface IParameterCapturingPipelineCallbacks { public void CapturingStart(StartCapturingParametersPayload request, IList methods); - public void CapturingStop(Guid RequestId); - public void FailedToCapture(Guid RequestId, ParameterCapturingEvents.CapturingFailedReason Reason, string Details); + public void CapturingStop(Guid requestId); + public void FailedToCapture(Guid requestId, ParameterCapturingEvents.CapturingFailedReason reason, string details); + public void ProbeFault(Guid requestId, InstrumentedMethod faultingMethod); } } diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs index dbc6673f07c..6a303e41e2a 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs @@ -51,15 +51,23 @@ public async Task RunAsync(CancellationToken stoppingToken) { CapturingRequest request = await _requestQueue.Reader.ReadAsync(stoppingToken); - void onFault(object? sender, ulong uniquifier) + void onFault(object? sender, InstrumentedMethod faultingMethod) { - _ = request.StopRequest.TrySetResult(); + _callbacks.ProbeFault(request.Payload.RequestId, faultingMethod); } - _probeManager.OnProbeFault += onFault; + using ScopeGuard sg = new( + initialize: () => + { + _probeManager.OnProbeFault += onFault; + }, + uninitialize: () => + { + _probeManager.OnProbeFault -= onFault; + }); + if (!await TryStartCapturingAsync(request.Payload, stoppingToken).ConfigureAwait(false)) { - _probeManager.OnProbeFault -= onFault; continue; } @@ -75,7 +83,6 @@ void onFault(object? sender, ulong uniquifier) } - _probeManager.OnProbeFault -= onFault; await _probeManager.StopCapturingAsync(stoppingToken).ConfigureAwait(false); _callbacks.CapturingStop(request.Payload.RequestId); diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs new file mode 100644 index 00000000000..6d1e1a4b570 --- /dev/null +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs @@ -0,0 +1,28 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.Diagnostics.Monitoring.StartupHook; +using System; + +namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.Pipeline +{ + public sealed class ScopeGuard : IDisposable + { + private long _disposedState; + private readonly Action _uninitialize; + + public ScopeGuard(Action initialize, Action uninitialize) + { + initialize(); + _uninitialize = uninitialize; + } + + public void Dispose() + { + if (!DisposableHelper.CanDispose(ref _disposedState)) + return; + + _uninitialize(); + } + } +} diff --git a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h index 6e57ec196fa..e238ca6f929 100644 --- a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h +++ b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h @@ -53,7 +53,8 @@ enum class ManagedCallbackType typedef struct _MANAGED_CALLBACK_REQUEST { ProbeWorkerInstruction instruction; - union { + union + { HRESULT hr; FunctionID functionId; } payload; diff --git a/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs b/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs index d93dd287f53..7e20555bdf0 100644 --- a/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs +++ b/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs @@ -23,7 +23,7 @@ internal sealed class TestFunctionProbesManager : IFunctionProbesManager private readonly Action> _onStart; private readonly Action _onStop; - public event EventHandler OnProbeFault; + public event EventHandler OnProbeFault; public TestFunctionProbesManager(Action> onStart = null, Action onStop = null) { @@ -33,7 +33,14 @@ public TestFunctionProbesManager(Action> onStart = null, Actio public void TriggerFault(ulong uniquifier) { - OnProbeFault?.Invoke(this, uniquifier); + var methodCache = FunctionProbesStub.InstrumentedMethodCache; + if (methodCache == null || + !methodCache.TryGetValue(uniquifier, out InstrumentedMethod instrumentedMethod)) + { + throw new ArgumentException(nameof(uniquifier)); + } + + OnProbeFault?.Invoke(this, instrumentedMethod); } public Task StartCapturingAsync(IList methods, CancellationToken token) @@ -58,15 +65,18 @@ internal sealed class TestParameterCapturingCallbacks : IParameterCapturingPipel private readonly Action> _onCapturingStart; private readonly Action _onCapturingStop; private readonly Action _onCapturingFailed; + private readonly Action _onProbeFault; public TestParameterCapturingCallbacks( Action> onCapturingStart = null, Action onCapturingStop = null, - Action onCapturingFailed = null) + Action onCapturingFailed = null, + Action onProbeFault = null) { _onCapturingStart = onCapturingStart; _onCapturingStop = onCapturingStop; _onCapturingFailed = onCapturingFailed; + _onProbeFault = onProbeFault; } public void CapturingStart(StartCapturingParametersPayload request, IList methods) @@ -74,14 +84,19 @@ public void CapturingStart(StartCapturingParametersPayload request, IList onStopCallbackSource = new(TaskCreationOptions.RunContinuationsAsynchronously); TaskCompletionSource<(Guid, IList)> onStartCallbackSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + TaskCompletionSource<(Guid, InstrumentedMethod)> onProbeFaultCallbackSource = new(TaskCreationOptions.RunContinuationsAsynchronously); using IDisposable registration = cts.Token.Register(() => { _ = onStartCallbackSource.TrySetCanceled(cts.Token); - _ = onStopCallbackSource.TrySetCanceled(cts.Token); + _ = onProbeFaultCallbackSource.TrySetCanceled(cts.Token); }); TestFunctionProbesManager probeManager = new(); @@ -297,9 +312,9 @@ public async Task Request_StopsAndNotifiesAfterFault() { onStartCallbackSource.TrySetResult((payload.RequestId, methods)); }, - onCapturingStop: (requestId) => + onProbeFault: (requestId, faultingMethod) => { - onStopCallbackSource.TrySetResult(requestId); + onProbeFaultCallbackSource.TrySetResult((requestId, faultingMethod)); }); ParameterCapturingPipeline pipeline = new(probeManager, callbacks); @@ -315,8 +330,9 @@ public async Task Request_StopsAndNotifiesAfterFault() probeManager.TriggerFault(instrumentedMethod.GetFunctionId()); // Assert - Guid stoppedRequest = await onStopCallbackSource.Task; - Assert.Equal(payload.RequestId, stoppedRequest); + (Guid faultingRequest, InstrumentedMethod faultingMethod) = await onProbeFaultCallbackSource.Task; + Assert.Equal(payload.RequestId, faultingRequest); + Assert.Equal(instrumentedMethod.GetFunctionId(), faultingMethod.FunctionId); } [Fact] diff --git a/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs b/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs index bd152b665df..f7d30f6c74a 100644 --- a/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs +++ b/src/Tests/Microsoft.Diagnostics.Monitoring.UnitTestApp/Scenarios/FunctionProbes/FunctionProbesScenario.cs @@ -234,10 +234,10 @@ private static async Task Test_ExceptionThrownByProbeAsync(FunctionProbesManager await probeManager.StartCapturingAsync(new[] { method }, token); - TaskCompletionSource faultingUniquifierSource = new(TaskCreationOptions.RunContinuationsAsynchronously); - void onFault(object caller, ulong uniquifier) + TaskCompletionSource faultingMethodSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + void onFault(object caller, InstrumentedMethod faultingMethod) { - _ = faultingUniquifierSource.TrySetResult(uniquifier); + _ = faultingMethodSource.TrySetResult(faultingMethod); } probeManager.OnProbeFault += onFault; @@ -246,8 +246,8 @@ void onFault(object caller, ulong uniquifier) // Faults are handled asynchronously, so wait for the event to propagate try { - ulong faultingUniquifier = await faultingUniquifierSource.Task.WaitAsync(token); - Assert.Equal(method.GetFunctionId(), faultingUniquifier); + InstrumentedMethod faultingMethod = await faultingMethodSource.Task.WaitAsync(token); + Assert.Equal(method.GetFunctionId(), faultingMethod.FunctionId); } finally { From ec22ca70ffdd7aa4f8272bfeb3ea1c7381bbd10d Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 13:50:48 -0700 Subject: [PATCH 04/21] Cleanup comment --- .../FunctionProbes/IFunctionProbesManager.cs | 1 - .../ProbeInstrumentation/ProbeInstrumentation.cpp | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs index c530da79aea..321a4bd3f9f 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/IFunctionProbesManager.cs @@ -15,7 +15,6 @@ internal interface IFunctionProbesManager : IDisposable public Task StopCapturingAsync(CancellationToken token); - public event EventHandler OnProbeFault; } } diff --git a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp index 03959e7ceb8..6b9d4ce7f56 100644 --- a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp +++ b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp @@ -65,12 +65,9 @@ HRESULT ProbeInstrumentation::InitBackgroundService() m_probeManagementThread = thread(&ProbeInstrumentation::WorkerThread, this); // // Create a dedicated thread for managed callbacks. - // Performing the callbacks will taint the calling thread, - // preventing it from using certain ICorProfiler APIs marked as "unsafe". - // - // The unsafe ICorProfiler APIs will believe our thread is calling them asynchronously, - // failing the request with CORPROF_E_UNSUPPORTED_CALL_SEQUENCE, - // even if our thread has already been initialized with ICorProfiler. + // Performing the callbacks will taint the calling thread preventing it + // from using certain ICorProfiler APIs marked as unsafe. + // Those functions will fail with CORPROF_E_UNSUPPORTED_CALL_SEQUENCE. // m_managedCallbackThread = thread(&ProbeInstrumentation::ManagedCallbackThread, this); return S_OK; From 78ba1a6a27dc8722784521fdb2cce2861d895ad4 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 13:54:27 -0700 Subject: [PATCH 05/21] Fix fault test --- .../Pipeline/ParameterCapturingPipelineTests.cs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs b/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs index 7e20555bdf0..6beaf7e38cf 100644 --- a/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs +++ b/src/Tests/Microsoft.Diagnostics.Monitoring.HostingStartup.UnitTests/ParameterCapturing/Pipeline/ParameterCapturingPipelineTests.cs @@ -31,16 +31,10 @@ public TestFunctionProbesManager(Action> onStart = null, Actio _onStop = onStop; } - public void TriggerFault(ulong uniquifier) + public void TriggerFault(MethodInfo method) { - var methodCache = FunctionProbesStub.InstrumentedMethodCache; - if (methodCache == null || - !methodCache.TryGetValue(uniquifier, out InstrumentedMethod instrumentedMethod)) - { - throw new ArgumentException(nameof(uniquifier)); - } - - OnProbeFault?.Invoke(this, instrumentedMethod); + InstrumentedMethod faultingMethod = new(method, BoxingTokens.GetBoxingTokens(method)); + OnProbeFault?.Invoke(this, faultingMethod); } public Task StartCapturingAsync(IList methods, CancellationToken token) @@ -327,7 +321,7 @@ public async Task ProbeFault_DoesNotify() MethodInfo instrumentedMethod = Assert.Single(methods); // Act - probeManager.TriggerFault(instrumentedMethod.GetFunctionId()); + probeManager.TriggerFault(instrumentedMethod); // Assert (Guid faultingRequest, InstrumentedMethod faultingMethod) = await onProbeFaultCallbackSource.Task; From 3080efd82aa6ef47d5dcd9012e068a817b73b93c Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 14:07:15 -0700 Subject: [PATCH 06/21] Add dedicated error message for dynamics --- .../FunctionProbes/FunctionProbesManager.cs | 2 +- .../ParameterCapturing/ParameterCapturingService.cs | 2 +- .../ParameterCapturingStrings.Designer.cs | 11 ++++++++++- .../ParameterCapturing/ParameterCapturingStrings.resx | 5 ++++- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 666b4205875..d234f274c85 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -203,7 +203,7 @@ public async Task StartCapturingAsync(IList methods, CancellationTok ulong functionId = method.GetFunctionId(); if (functionId == 0) { - throw new NotSupportedException(method.Name); + throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, ParameterCapturingStrings.ErrorMessage_FunctionDoesNotHaveIdFormatString, method.Name)); } uint[] methodBoxingTokens = BoxingTokens.GetBoxingTokens(method); diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs index 4a58ea2a5b1..5d403143c80 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs @@ -96,7 +96,7 @@ public void FailedToCapture(Guid requestId, ParameterCapturingEvents.CapturingFa public void ProbeFault(Guid requestId, InstrumentedMethod faultingMethod) { // TODO: Report back this fault on ParameterCapturingEventSource. - _logger?.LogInformation(ParameterCapturingStrings.StoppingParameterCapturingDueToProbeFault, faultingMethod.MethodWithParametersTemplateString); + _logger?.LogWarning(ParameterCapturingStrings.StoppingParameterCapturingDueToProbeFault, faultingMethod.MethodWithParametersTemplateString); try { diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs index e39d101e3ad..60ff990f7a4 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs @@ -61,6 +61,15 @@ internal ParameterCapturingStrings() { } } + /// + /// Looks up a localized string similar to Method '{0}' is unsupported, it does not have a FunctionID.. + /// + internal static string ErrorMessage_FunctionDoesNotHaveIdFormatString { + get { + return ResourceManager.GetString("ErrorMessage_FunctionDoesNotHaveIdFormatString", resourceCulture); + } + } + /// /// Looks up a localized string similar to Mismatch probe state. Expected '{0}', was '{1}'.. /// @@ -134,7 +143,7 @@ internal static string StopParameterCapturing { } /// - /// Looks up a localized string similar to Parameter capturing encountered an internal exception when processing '{method}', stopping.. + /// Looks up a localized string similar to Parameter capturing encountered an internal error when processing '{method}', stopping.. /// internal static string StoppingParameterCapturingDueToProbeFault { get { diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx index 77e7ce5c265..b4760c9a027 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx @@ -117,6 +117,9 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + Method '{0}' is unsupported, it does not have a FunctionID. + Mismatch probe state. Expected '{0}', was '{1}'. @@ -142,7 +145,7 @@ Stopped parameter capturing. - Parameter capturing encountered an internal exception when processing '{method}', stopping. + Parameter capturing encountered an internal error when processing '{method}', stopping. this From a502934d9e2febcdb11c2a91ac309c470001c9f0 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 14:07:37 -0700 Subject: [PATCH 07/21] Cleanup error messages --- .../ParameterCapturing/ParameterCapturingStrings.Designer.cs | 2 +- .../ParameterCapturing/ParameterCapturingStrings.resx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs index 60ff990f7a4..6084fc76e22 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.Designer.cs @@ -62,7 +62,7 @@ internal ParameterCapturingStrings() { } /// - /// Looks up a localized string similar to Method '{0}' is unsupported, it does not have a FunctionID.. + /// Looks up a localized string similar to Method '{0}' is unsupported, it does not have a function id.. /// internal static string ErrorMessage_FunctionDoesNotHaveIdFormatString { get { diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx index b4760c9a027..b81f9fecdcd 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingStrings.resx @@ -118,7 +118,7 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - Method '{0}' is unsupported, it does not have a FunctionID. + Method '{0}' is unsupported, it does not have a function id. Mismatch probe state. Expected '{0}', was '{1}'. From fe6a931d81840acd7c45ac436fe7486f5ab208e4 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 14:18:36 -0700 Subject: [PATCH 08/21] nit: spelling --- cspell.json | 1 + .../ParameterCapturing/FunctionProbes/FunctionProbesManager.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cspell.json b/cspell.json index db5715bd42f..dbc68d0c3e0 100644 --- a/cspell.json +++ b/cspell.json @@ -98,6 +98,7 @@ "trce", "tstr", "ukwn", + "uninitialize", "Uninstallation", "uniquifier", "Unlocalized", diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index d234f274c85..c00bc34c21a 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -101,7 +101,7 @@ private void OnFault(ulong uniquifier) !methodCache.TryGetValue(uniquifier, out InstrumentedMethod? instrumentedMethod)) { // - // The probe fault occured in a method that is no longer actively instrumented, ignore. + // The probe fault occurred in a method that is no longer actively instrumented, ignore. // This can happen when we request uninstallation of function probes and there's still a thread // actively in one of the instrumented methods and it happens to fault. // From 71234c5f4c2783f56a0e39a2eda043c79129e21a Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 10 Aug 2023 14:40:17 -0700 Subject: [PATCH 09/21] Min diff --- .../ParameterCapturing/FunctionProbes/LogEmittingProbes.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs index bb4836c464d..5427e7610e2 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/LogEmittingProbes.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using Microsoft.Extensions.Logging; + namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.FunctionProbes { internal sealed class LogEmittingProbes : IFunctionProbes From 9fffe7f8cc5e76297ee32ebadef0ebc09ae1cef7 Mon Sep 17 00:00:00 2001 From: Joe Schmitt <1146681+schmittjoseph@users.noreply.github.com> Date: Wed, 16 Aug 2023 10:13:14 -0700 Subject: [PATCH 10/21] Update src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h --- .../ProbeInstrumentation/ProbeInstrumentation.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h index e238ca6f929..b86e491f72c 100644 --- a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h +++ b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.h @@ -42,14 +42,6 @@ typedef struct _PROBE_WORKER_PAYLOAD std::vector requests; } PROBE_WORKER_PAYLOAD; -enum class ManagedCallbackType -{ - REGISTER_PROBE, - INSTALL_PROBES, - UNINSTALL_PROBES, - FAULTING_PROBE -}; - typedef struct _MANAGED_CALLBACK_REQUEST { ProbeWorkerInstruction instruction; From 25c31cc9b78ec8fa7cec5397d5a07373bba4916e Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 10:28:50 -0700 Subject: [PATCH 11/21] PR feedback --- .../FunctionProbes/FunctionProbesManager.cs | 11 +++-- .../Pipeline/ParameterCapturingPipeline.cs | 47 +++++++++---------- .../ParameterCapturing/Pipeline/ScopeGuard.cs | 28 ----------- 3 files changed, 31 insertions(+), 55 deletions(-) delete mode 100644 src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index c00bc34c21a..7b7854cd27f 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -138,16 +138,18 @@ private void StateMismatch(long expected) InvalidOperationException ex = new(string.Format(CultureInfo.InvariantCulture, ParameterCapturingStrings.ErrorMessage_ProbeStateMismatchFormatString, expected, _probeState)); _probeState = ProbeStateUnrecoverable; + _ = _probeRegistrationTaskSource.TrySetException(ex); _ = _installationTaskSource?.TrySetException(ex); _ = _uninstallationTaskSource?.TrySetException(ex); - _ = _probeRegistrationTaskSource?.TrySetException(ex); } public async Task StopCapturingAsync(CancellationToken token) { + DisposableHelper.ThrowIfDisposed(ref _disposedState); + if (ProbeStateInstalled != Interlocked.CompareExchange(ref _probeState, ProbeStateUninstalling, ProbeStateInstalled)) { - throw new InvalidOperationException(); + throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, ParameterCapturingStrings.ErrorMessage_ProbeStateMismatchFormatString, ProbeStateInstalled, _probeState)); } _uninstallationTaskSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -179,6 +181,8 @@ private void StopCapturingCore() public async Task StartCapturingAsync(IList methods, CancellationToken token) { + DisposableHelper.ThrowIfDisposed(ref _disposedState); + if (methods.Count == 0) { throw new ArgumentException(nameof(methods)); @@ -188,7 +192,7 @@ public async Task StartCapturingAsync(IList methods, CancellationTok if (ProbeStateUninstalled != Interlocked.CompareExchange(ref _probeState, ProbeStateInstalling, ProbeStateUninstalled)) { - throw new InvalidOperationException(); + throw new InvalidOperationException(string.Format(CultureInfo.InvariantCulture, ParameterCapturingStrings.ErrorMessage_ProbeStateMismatchFormatString, ProbeStateUninstalled, _probeState)); } try @@ -245,6 +249,7 @@ public void Dispose() FunctionProbesStub.Instance = null; + _ = _probeRegistrationTaskSource.TrySetCanceled(); _ = _installationTaskSource?.TrySetCanceled(); _ = _uninstallationTaskSource?.TrySetCanceled(); diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs index 6a303e41e2a..891eeb59434 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs @@ -56,37 +56,36 @@ void onFault(object? sender, InstrumentedMethod faultingMethod) _callbacks.ProbeFault(request.Payload.RequestId, faultingMethod); } - using ScopeGuard sg = new( - initialize: () => + try + { + _probeManager.OnProbeFault += onFault; + + if (!await TryStartCapturingAsync(request.Payload, stoppingToken).ConfigureAwait(false)) { - _probeManager.OnProbeFault += onFault; - }, - uninitialize: () => + continue; + } + + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken); + cts.CancelAfter(request.Payload.Duration); + + try + { + await request.StopRequest.Task.WaitAsync(cts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) { - _probeManager.OnProbeFault -= onFault; - }); - - if (!await TryStartCapturingAsync(request.Payload, stoppingToken).ConfigureAwait(false)) - { - continue; - } - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken); - cts.CancelAfter(request.Payload.Duration); + } - try - { - await request.StopRequest.Task.WaitAsync(cts.Token).ConfigureAwait(false); + await _probeManager.StopCapturingAsync(stoppingToken).ConfigureAwait(false); + + _callbacks.CapturingStop(request.Payload.RequestId); + _ = _allRequests.TryRemove(request.Payload.RequestId, out _); } - catch (OperationCanceledException) + finally { - + _probeManager.OnProbeFault -= onFault; } - - await _probeManager.StopCapturingAsync(stoppingToken).ConfigureAwait(false); - - _callbacks.CapturingStop(request.Payload.RequestId); - _ = _allRequests.TryRemove(request.Payload.RequestId, out _); } } diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs deleted file mode 100644 index 6d1e1a4b570..00000000000 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ScopeGuard.cs +++ /dev/null @@ -1,28 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using Microsoft.Diagnostics.Monitoring.StartupHook; -using System; - -namespace Microsoft.Diagnostics.Monitoring.HostingStartup.ParameterCapturing.Pipeline -{ - public sealed class ScopeGuard : IDisposable - { - private long _disposedState; - private readonly Action _uninitialize; - - public ScopeGuard(Action initialize, Action uninitialize) - { - initialize(); - _uninitialize = uninitialize; - } - - public void Dispose() - { - if (!DisposableHelper.CanDispose(ref _disposedState)) - return; - - _uninitialize(); - } - } -} From fa7d70dae303b57861e3acd34bfde329cf823a8c Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 10:29:46 -0700 Subject: [PATCH 12/21] PR feedback --- .../ProbeInstrumentation/ProbeInstrumentation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp index 6b9d4ce7f56..ede3ede5ff4 100644 --- a/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp +++ b/src/Profilers/MutatingMonitorProfiler/ProbeInstrumentation/ProbeInstrumentation.cpp @@ -215,7 +215,7 @@ void ProbeInstrumentation::ShutdownBackgroundService() { DisableIncomingRequests(); m_managedCallbackQueue.Complete(); - m_probeManagementThread.join(); + m_managedCallbackThread.join(); m_probeManagementThread.join(); } From 26ea76ae17629d5d8aeaaa7fa642d89da8a44912 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 15:37:25 -0700 Subject: [PATCH 13/21] Fix merge --- .../ParameterCapturing/ParameterCapturingService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs index 14e4dec0d95..2cacd983b52 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/ParameterCapturingService.cs @@ -48,7 +48,7 @@ public ParameterCapturingService(IServiceProvider services) IpcCommand.StopCapturingParameters, OnStopMessage); - ILogger serviceLogger = services.GetService>() + _logger = services.GetService>() ?? throw new NotSupportedException(ParameterCapturingStrings.FeatureUnsupported_NoLogger); ILogger userLogger = services.GetService>() From 2bdda7898c69b05cc78d1b9704d71e93acd8c757 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 16:09:41 -0700 Subject: [PATCH 14/21] PR feedback --- .../FunctionProbes/FunctionProbesManager.cs | 83 ++++++++++++++++--- 1 file changed, 71 insertions(+), 12 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 7b7854cd27f..0019ea711e5 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -36,14 +36,19 @@ private static extern void RequestFunctionProbeInstallation( [DllImport(ProfilerIdentifiers.MutatingProfiler.LibraryRootFileName, CallingConvention = CallingConvention.StdCall, PreserveSig = false)] private static extern void RegisterFunctionProbeCallbacks( - FunctionProbeRegistrationCallback onRegistration, - FunctionProbeInstallationCallback onInstallation, - FunctionProbeUninstallationCallback onUninstallation, - FunctionProbeFaultCallback onFault); + IntPtr onRegistration, + IntPtr onInstallation, + IntPtr onUninstallation, + IntPtr onFault); [DllImport(ProfilerIdentifiers.MutatingProfiler.LibraryRootFileName, CallingConvention = CallingConvention.StdCall, PreserveSig = false)] private static extern void UnregisterFunctionProbeCallbacks(); + private readonly FunctionProbeRegistrationCallback _onRegistrationDelegate; + private readonly FunctionProbeInstallationCallback _onInstallationDelegate; + private readonly FunctionProbeUninstallationCallback _onUninstallationDelegate; + private readonly FunctionProbeFaultCallback _onFaultDelegate; + private long _probeState; private const long ProbeStateUninitialized = default(long); private const long ProbeStateUninstalled = 1; @@ -56,6 +61,7 @@ private static extern void RegisterFunctionProbeCallbacks( private TaskCompletionSource? _installationTaskSource; private TaskCompletionSource? _uninstallationTaskSource; + private readonly CancellationTokenSource _disposalTokenSource = new(); private long _disposedState; public event EventHandler? OnProbeFault; @@ -64,7 +70,17 @@ public FunctionProbesManager(IFunctionProbes probes) { ProfilerResolver.InitializeResolver(); - RegisterFunctionProbeCallbacks(OnRegistration, OnInstallation, OnUninstallation, OnFault); + _onRegistrationDelegate = OnRegistration; + _onInstallationDelegate = OnInstallation; + _onUninstallationDelegate = OnUninstallation; + _onFaultDelegate = OnFault; + + RegisterFunctionProbeCallbacks( + Marshal.GetFunctionPointerForDelegate(_onRegistrationDelegate), + Marshal.GetFunctionPointerForDelegate(_onInstallationDelegate), + Marshal.GetFunctionPointerForDelegate(_onUninstallationDelegate), + Marshal.GetFunctionPointerForDelegate(_onFaultDelegate)); + RequestFunctionProbeRegistration(FunctionProbesStub.GetProbeFunctionId()); FunctionProbesStub.Instance = probes; @@ -164,7 +180,12 @@ public async Task StopCapturingAsync(CancellationToken token) throw; } - await _uninstallationTaskSource.Task.WaitAsync(token).ConfigureAwait(false); + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalTokenSource.Token, token); + using IDisposable _ = cts.Token.Register(() => + { + _uninstallationTaskSource.TrySetCanceled(cts.Token); + }); + await _uninstallationTaskSource.Task.ConfigureAwait(false); } private void StopCapturingCore() @@ -174,6 +195,7 @@ private void StopCapturingCore() return; } + _probeState = ProbeStateUninstalling; FunctionProbesStub.InstrumentedMethodCache = null; RequestFunctionProbeUninstallation(); } @@ -188,6 +210,7 @@ public async Task StartCapturingAsync(IList methods, CancellationTok throw new ArgumentException(nameof(methods)); } + // _probeRegistrationTaskSource will be cancelled (if needed) on dispose await _probeRegistrationTaskSource.Task.WaitAsync(token).ConfigureAwait(false); if (ProbeStateUninstalled != Interlocked.CompareExchange(ref _probeState, ProbeStateInstalling, ProbeStateUninstalled)) @@ -239,7 +262,25 @@ public async Task StartCapturingAsync(IList methods, CancellationTok throw; } - await _installationTaskSource.Task.WaitAsync(token).ConfigureAwait(false); + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalTokenSource.Token, token); + using IDisposable _ = cts.Token.Register(() => + { + if (!_installationTaskSource.TrySetCanceled(cts.Token)) + { + return; + } + + try + { + StopCapturingCore(); + } + catch + { + + } + }); + + await _installationTaskSource.Task.ConfigureAwait(false); } public void Dispose() @@ -247,20 +288,38 @@ public void Dispose() if (!DisposableHelper.CanDispose(ref _disposedState)) return; - FunctionProbesStub.Instance = null; + try + { + _disposalTokenSource.Cancel(); + } + catch + { + } - _ = _probeRegistrationTaskSource.TrySetCanceled(); - _ = _installationTaskSource?.TrySetCanceled(); - _ = _uninstallationTaskSource?.TrySetCanceled(); + _ = _probeRegistrationTaskSource.TrySetCanceled(_disposalTokenSource.Token); + _disposalTokenSource.Dispose(); try { UnregisterFunctionProbeCallbacks(); - StopCapturingCore(); } catch { } + + // We only need to trigger a probe uninstallation if they're succesfully installed. + // If the probes are in the middle of Installation on Dispose, StartAsync will + // take care of triggering an uninstallation. + if (_probeState == ProbeStateInstalled) + { + try + { + StopCapturingCore(); + } + catch + { + } + } } } } From a27f8ea7d0d2d830d1ae1516f46842855d73f57e Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 16:11:21 -0700 Subject: [PATCH 15/21] fix typo --- .../ParameterCapturing/FunctionProbes/FunctionProbesManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 0019ea711e5..85700c9e29a 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -307,7 +307,7 @@ public void Dispose() { } - // We only need to trigger a probe uninstallation if they're succesfully installed. + // We only need to trigger a probe uninstallation if they're successfully installed. // If the probes are in the middle of Installation on Dispose, StartAsync will // take care of triggering an uninstallation. if (_probeState == ProbeStateInstalled) From cbec645fdbe03fa9ae19f0790a504e19fc006b29 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 16:14:48 -0700 Subject: [PATCH 16/21] Fix comment --- .../ParameterCapturing/FunctionProbes/FunctionProbesManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 85700c9e29a..ce45fd77896 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -308,7 +308,7 @@ public void Dispose() } // We only need to trigger a probe uninstallation if they're successfully installed. - // If the probes are in the middle of Installation on Dispose, StartAsync will + // If the probes are in the middle of Installation on Dispose, StartCapturingAsync will // take care of triggering an uninstallation. if (_probeState == ProbeStateInstalled) { From 9364d96bed39933aec98caa0572d485ac1b172f3 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 16:23:14 -0700 Subject: [PATCH 17/21] Min diff --- .../FunctionProbes/FunctionProbesManager.cs | 30 ++----------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index ce45fd77896..d4318d0e0f4 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -195,7 +195,6 @@ private void StopCapturingCore() return; } - _probeState = ProbeStateUninstalling; FunctionProbesStub.InstrumentedMethodCache = null; RequestFunctionProbeUninstallation(); } @@ -265,19 +264,7 @@ public async Task StartCapturingAsync(IList methods, CancellationTok using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalTokenSource.Token, token); using IDisposable _ = cts.Token.Register(() => { - if (!_installationTaskSource.TrySetCanceled(cts.Token)) - { - return; - } - - try - { - StopCapturingCore(); - } - catch - { - - } + _installationTaskSource.TrySetCanceled(cts.Token); }); await _installationTaskSource.Task.ConfigureAwait(false); @@ -302,24 +289,11 @@ public void Dispose() try { UnregisterFunctionProbeCallbacks(); + StopCapturingCore(); } catch { } - - // We only need to trigger a probe uninstallation if they're successfully installed. - // If the probes are in the middle of Installation on Dispose, StartCapturingAsync will - // take care of triggering an uninstallation. - if (_probeState == ProbeStateInstalled) - { - try - { - StopCapturingCore(); - } - catch - { - } - } } } } From f4fc7123e991245c7cf412e7503bfc888aba4fce Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Wed, 16 Aug 2023 16:24:20 -0700 Subject: [PATCH 18/21] Min diff --- .../ParameterCapturing/FunctionProbes/FunctionProbesManager.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index d4318d0e0f4..685aa80381f 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -264,6 +264,8 @@ public async Task StartCapturingAsync(IList methods, CancellationTok using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalTokenSource.Token, token); using IDisposable _ = cts.Token.Register(() => { + // On cancellation, simply stop anyone waiting on it. + // The actual installation process isn't trivial to stop at this point. _installationTaskSource.TrySetCanceled(cts.Token); }); From 30daee1b786ae45b22477c32197c98714c72f722 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 17 Aug 2023 08:35:11 -0700 Subject: [PATCH 19/21] PR feedback --- .../FunctionProbes/FunctionProbesManager.cs | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 685aa80381f..bf54cccf881 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -62,6 +62,8 @@ private static extern void RegisterFunctionProbeCallbacks( private TaskCompletionSource? _uninstallationTaskSource; private readonly CancellationTokenSource _disposalTokenSource = new(); + private readonly CancellationToken _disposalToken; + private long _disposedState; public event EventHandler? OnProbeFault; @@ -70,6 +72,8 @@ public FunctionProbesManager(IFunctionProbes probes) { ProfilerResolver.InitializeResolver(); + _disposalToken = _disposalTokenSource.Token; + _onRegistrationDelegate = OnRegistration; _onInstallationDelegate = OnInstallation; _onUninstallationDelegate = OnUninstallation; @@ -180,7 +184,7 @@ public async Task StopCapturingAsync(CancellationToken token) throw; } - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalTokenSource.Token, token); + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalToken, token); using IDisposable _ = cts.Token.Register(() => { _uninstallationTaskSource.TrySetCanceled(cts.Token); @@ -261,11 +265,26 @@ public async Task StartCapturingAsync(IList methods, CancellationTok throw; } - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalTokenSource.Token, token); + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalToken, token); using IDisposable _ = cts.Token.Register(() => { - // On cancellation, simply stop anyone waiting on it. - // The actual installation process isn't trivial to stop at this point. + // + // We need to uninstall the probes ourselves here if dispose has happened otherwise the probes could be left in an installed state. + // + // NOTE: It's possible that StopCapturingCore could be called twice by doing this - once by Dispose and once by us, this is OK + // as the native layer will gracefully handle multiple RequestFunctionProbeUninstallation calls. + // + if (_disposalToken.IsCancellationRequested) + { + try + { + StopCapturingCore(); + } + catch + { + } + } + _installationTaskSource.TrySetCanceled(cts.Token); }); From 9f5ab237262ee25c506d548cb9616213eccb24a6 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 17 Aug 2023 14:57:35 -0700 Subject: [PATCH 20/21] PR feedback --- .../FunctionProbes/FunctionProbesManager.cs | 67 +++++++++++++------ .../Pipeline/ParameterCapturingPipeline.cs | 8 +++ 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index bf54cccf881..39a1c5794a6 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -74,6 +74,13 @@ public FunctionProbesManager(IFunctionProbes probes) _disposalToken = _disposalTokenSource.Token; + // + // CONSIDER: + // Use UnmanagedCallersOnlyAttribute for the callbacks + // and use the address-of operator to get a function pointer + // to give to the profiler. + // + _onRegistrationDelegate = OnRegistration; _onInstallationDelegate = OnInstallation; _onUninstallationDelegate = OnUninstallation; @@ -165,6 +172,10 @@ private void StateMismatch(long expected) public async Task StopCapturingAsync(CancellationToken token) { + // + // NOTE: Do not await anything until after StopCapturingCore is called + // to ensure deterministic cancellation behavior. + // DisposableHelper.ThrowIfDisposed(ref _disposedState); if (ProbeStateInstalled != Interlocked.CompareExchange(ref _probeState, ProbeStateUninstalling, ProbeStateInstalled)) @@ -185,11 +196,18 @@ public async Task StopCapturingAsync(CancellationToken token) } using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalToken, token); - using IDisposable _ = cts.Token.Register(() => + try + { + using IDisposable _ = cts.Token.Register(() => + { + _uninstallationTaskSource?.TrySetCanceled(cts.Token); + }); + await _uninstallationTaskSource.Task.ConfigureAwait(false); + } + finally { - _uninstallationTaskSource.TrySetCanceled(cts.Token); - }); - await _uninstallationTaskSource.Task.ConfigureAwait(false); + _uninstallationTaskSource = null; + } } private void StopCapturingCore() @@ -266,29 +284,36 @@ public async Task StartCapturingAsync(IList methods, CancellationTok } using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(_disposalToken, token); - using IDisposable _ = cts.Token.Register(() => + try { - // - // We need to uninstall the probes ourselves here if dispose has happened otherwise the probes could be left in an installed state. - // - // NOTE: It's possible that StopCapturingCore could be called twice by doing this - once by Dispose and once by us, this is OK - // as the native layer will gracefully handle multiple RequestFunctionProbeUninstallation calls. - // - if (_disposalToken.IsCancellationRequested) + using IDisposable _ = cts.Token.Register(() => { - try + // + // We need to uninstall the probes ourselves here if dispose has happened otherwise the probes could be left in an installed state. + // + // NOTE: It's possible that StopCapturingCore could be called twice by doing this - once by Dispose and once by us, this is OK + // as the native layer will gracefully handle multiple RequestFunctionProbeUninstallation calls. + // + if (_disposalToken.IsCancellationRequested) { - StopCapturingCore(); + try + { + StopCapturingCore(); + } + catch + { + } } - catch - { - } - } - _installationTaskSource.TrySetCanceled(cts.Token); - }); + _installationTaskSource?.TrySetCanceled(cts.Token); + }); - await _installationTaskSource.Task.ConfigureAwait(false); + await _installationTaskSource.Task.ConfigureAwait(false); + } + finally + { + _installationTaskSource = null; + } } public void Dispose() diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs index 891eeb59434..12827f66a57 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/Pipeline/ParameterCapturingPipeline.cs @@ -77,6 +77,14 @@ void onFault(object? sender, InstrumentedMethod faultingMethod) } + // + // NOTE: + // StopCapturingAsync will request a stop regardless of if the stoppingToken is set. + // While we don't support the host & services reloading, the above behavior will ensure + // that we don't leave the app in a potentially bad state on a reload. + // + // See: https://github.com/dotnet/dotnet-monitor/issues/5170 + // await _probeManager.StopCapturingAsync(stoppingToken).ConfigureAwait(false); _callbacks.CapturingStop(request.Payload.RequestId); From 70aa7c236d115cc5ebe6f89cd7d7c78a1f140e61 Mon Sep 17 00:00:00 2001 From: Joe Schmitt Date: Thu, 17 Aug 2023 14:59:28 -0700 Subject: [PATCH 21/21] Remove extra null checks --- .../FunctionProbes/FunctionProbesManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs index 39a1c5794a6..21093f378c2 100644 --- a/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs +++ b/src/Microsoft.Diagnostics.Monitoring.HostingStartup/ParameterCapturing/FunctionProbes/FunctionProbesManager.cs @@ -200,7 +200,7 @@ public async Task StopCapturingAsync(CancellationToken token) { using IDisposable _ = cts.Token.Register(() => { - _uninstallationTaskSource?.TrySetCanceled(cts.Token); + _uninstallationTaskSource.TrySetCanceled(cts.Token); }); await _uninstallationTaskSource.Task.ConfigureAwait(false); } @@ -305,7 +305,7 @@ public async Task StartCapturingAsync(IList methods, CancellationTok } } - _installationTaskSource?.TrySetCanceled(cts.Token); + _installationTaskSource.TrySetCanceled(cts.Token); }); await _installationTaskSource.Task.ConfigureAwait(false);