Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Proposal: Cancellable/Timeout-able DiagnosticClient methods #2151

Closed
jander-msft opened this issue Apr 6, 2021 · 0 comments · Fixed by #2350
Closed

API Proposal: Cancellable/Timeout-able DiagnosticClient methods #2151

jander-msft opened this issue Apr 6, 2021 · 0 comments · Fixed by #2350
Assignees
Milestone

Comments

@jander-msft
Copy link
Member

jander-msft commented Apr 6, 2021

Background and Motivation

The current methods on the DiagnosticsClient class only allow for non-async calls and are not cancellable / not able to timeout. This becomes problematic if a client attempts to interact with a runtime instance that is stalled/debugged/misbehaving because the method calls will block until you either kill your client process or abort the thread from which the client is calling the method.

The following is an example of the GetProcessEnvironment being called for a process that is being debugged and it hangs in IpcHeader.TryParse indefinitely:

Microsoft.Diagnostics.NETCore.Client.dll!Microsoft.Diagnostics.NETCore.Client.IpcHeader.TryParse(System.IO.BinaryReader reader) Line 58	C#
Microsoft.Diagnostics.NETCore.Client.dll!Microsoft.Diagnostics.NETCore.Client.IpcMessage.Parse(System.IO.Stream stream, System.Threading.CancellationToken token) Line 118	C#
Microsoft.Diagnostics.NETCore.Client.dll!Microsoft.Diagnostics.NETCore.Client.IpcClient.Read(System.IO.Stream stream, System.Threading.CancellationToken token) Line 69	C#
Microsoft.Diagnostics.NETCore.Client.dll!Microsoft.Diagnostics.NETCore.Client.IpcClient.SendMessage(Microsoft.Diagnostics.NETCore.Client.IpcEndpoint endpoint, Microsoft.Diagnostics.NETCore.Client.IpcMessage message, out Microsoft.Diagnostics.NETCore.Client.IpcMessage response) Line 45	C#
Microsoft.Diagnostics.NETCore.Client.dll!Microsoft.Diagnostics.NETCore.Client.DiagnosticsClient.GetProcessEnvironment() Line 214	C#

The can be problematic for applications such as dotnet-monitor. While the client of dotnet-monitor can cancel the HTTP request, the handler will still be waiting indefinitely for the misbehaving / frozen process to respond. Over time, this could at the very least build up thread exhaustion.

Proposed API Changes

public sealed class DiagnosticsClient
{
    public EventPipeSession StartEventPipeSession(IEnumerable<EventPipeProvider> providers, bool requestRundown=true, int circularBufferMB=256);
+   public EventPipeSession StartEventPipeSession(IEnumerable<EventPipeProvider> providers, TimeSpan timeout, bool requestRundown=true, int circularBufferMB=256);
+   public Task<EventPipeSession> StartEventPipeSessionAsync(IEnumerable<EventPipeProvider> providers, CancellationToken token, bool requestRundown=true, int circularBufferMB=256);
    public EventPipeSession StartEventPipeSession(EventPipeProvider provider, bool requestRundown=true, int circularBufferMB=256);
+   public EventPipeSession StartEventPipeSession(EventPipeProvider provider, TimeSpan timeout, bool requestRundown=true, int circularBufferMB=256);
+   public Task<EventPipeSession> StartEventPipeSessionAsync(EventPipeProvider provider, CancellationToken token, bool requestRundown=true, int circularBufferMB=256);
    public void WriteDump(DumpType dumpType, string dumpPath, bool logDumpGeneration=false);
+   public void WriteDump(DumpType dumpType, string dumpPath, TimeSpan timeout, bool logDumpGeneration=false);
+   public Task WriteDumpAsync(DumpType dumpType, string dumpPath, CancellationToken token, bool logDumpGeneration=false);
    public void AttachProfiler(TimeSpan attachTimeout, Guid profilerGuid, string profilerPath, byte[] additionalData=null);
+   public void AttachProfiler(TimeSpan attachTimeout, Guid profilerGuid, string profilerPath, TimeSpan timeout, byte[] additionalData=null);
+   public Task AttachProfilerAsync(TimeSpan attachTimeout, Guid profilerGuid, string profilerPath, CancellationToken token, byte[] additionalData=null);
    internal void ResumeRuntime();
+   internal void ResumeRuntime(TimeSpan timeout);
+   internal Task ResumeRuntimeAsync(CancellationToken token);
    internal ProcessInfo GetProcessInfo();
+   internal ProcessInfo GetProcessInfo(TimeSpan timeout);
+   internal Task<ProcessInfo> GetProcessInfoAsync(CancellationToken token);
    public Dictionary<string,string> GetProcessEnvironment();
+   public Dictionary<string,string> GetProcessEnvironment(TimeSpan timeout);
+   public Task<Dictionary<string,string>> GetProcessEnvironmentAsync(CancellationToken token);
}

Usage Examples

DiagnosticsClient client = new(<pid>);
using CancellationTokenSource cancellation = new(TimeSpan.FromSeconds(10));
var environment = await client.GetProcessEnvironmentAsync(cancellation.Token);

Details

The overloads with the new TimeSpan parameter should throw TimeoutException if the timeout expires. The overloads with the new CancellationToken exception should throw OperationCancelledException if the token is signalled.

Because of how stream reading works, the cancellation would have to dispose/close the stream to force the waiting reads to be cancelled. This shouldn't be too much of a problem since the caller to DiagnosticsClient doesn't have direct access to the stream. However, it would require handle ObjectDisposeException and throwing either TimeoutException or OperationCancelledException as appropriate.

The non-async overload for AttachProfiler could easily be misunderstood because there are two TimeSpan parameters. The original parameter value is serialized and sent to the other process to allow the runtime to cancel profiler attach within a certain amount of time. The new parameter is the one that would be used for cancelling the entire AttachProfiler operation. May need to thinking about the API usage here a bit more.

cc @josalem

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants