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]: Let accessing the instance of Exception (if any) on stack. #89724

Open
voroninp opened this issue Jul 31, 2023 · 15 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@voroninp
Copy link

voroninp commented Jul 31, 2023

Background and motivation

From time to time there's a need to not only check whether stack contains a bubbling up exception, but also to get access to the exception itself.

For example, in Open Telemetry library ExceptionProcessor uses Marshal.GetExceptionPointers to check whether exception is thrown withing the scope of activity.

However, if I don't want `OperationCancelledException' to be treated as an error, I need to have access to the instance of the exception thrown. Currently, there's no such possibility.

API Proposal

Well, I am not familiar with the intricacies of CLR, and whether such API is conceptually possible, but something like adding property to AppDomain could work:

public sealed class AppDomain
{
    public Exception? CurrentException {get;}
}

throw should set this property (overwriting previous value), catch with matching exception should set it to null.

API Usage

try
{
    throw new Exception();
}
finally
{
    // Here it is not null;
      Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);
}
try
{
    throw new Exception();
}
catch
{
    // Here it is null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is null);
}
finally
{
    // Here it is null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is null);
}
try
{
    throw new Exception();
}
catch
{
    // Here it is null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is null);

   throw;
}
finally
{
    // Here it is not null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);
}

Not sure whether value should propagate with async.

Task CheckMe() => Task.Run(() => Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null));
try
{
    throw new Exception();
}
finally
{
    // Here it is not null;
    Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);

    // will it output true or false? 
    await CheckMe();

    // Here it is not null;
    Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);
}
@voroninp voroninp added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 31, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 31, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 31, 2023
@teo-tsirpanis teo-tsirpanis added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 31, 2023
@ghost
Copy link

ghost commented Jul 31, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

From time to time there's a need to not only check whether stack contains a bubbling up exception, but also to get access to the exception itself.

For example, in Open Telemetry library ExceptionProcessor uses Marshal.GetExceptionPointers to check whether exception is thrown withing the scope of activity.

However, if I don't want `OperationCancelledException' to be treated as an error, I need to have access to the instance of the exception thrown. Currently, there's no such possibility.

API Proposal

Well, I am not familiar with the intricacies of CLR, and whether such API is conceptually possible, but something like adding property to AppDomain could work:

public sealed class AppDomain
{
    public Exception? CurrentException {get;}
}

throw should set this property (overwriting previous value), catch with matching exception should set it to null.

API Usage

try
{
    throw new Exception();
}
finally
{
    // Here it is not null;
      Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);
}
try
{
    throw new Exception();
}
catch
{
    // Here it is null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is null);
}
finally
{
    // Here it is null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is null);
}
try
{
    throw new Exception();
}
catch
{
    // Here it is null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is null);

   throw;
}
finally
{
    // Here it is not null;
   Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);
}

Not sure whether value should propagate with async.

Task CheckMe() => Task.Run(() => Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null));
try
{
    throw new Exception();
}
finally
{
    // Here it is not null;
    Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);

    // will it output true or false? 
    await CheckMe();

    // Here it is not null;
    Console.WriteLine(AppDomain.CurrentDomain.CurrentException is not null);
}
Author: voroninp
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@grbell-ms
Copy link
Member

Could Open Telemetry achieve the same result using a first-chance exception handler?
Whenever an exception is thrown, it could be stored in a thread/task -local field. The ExceptionProcessor could then access the most recent exception using the field and determine whether that exception is bubbling up using fnGetExceptionPointers.

@voroninp
Copy link
Author

voroninp commented Aug 2, 2023

@grbell-ms

If I am not missing something particularly important, it should work.

async Task Main()
{
    using var activity1 = new Activity();
    try
    {
        using var activity2 = new Activity();
	await ThrowAsync();
    }
    catch
    {		
    }
}

public Task ThrowAsync() => Task.Run(() => throw new OperationCanceledException());

public sealed class Activity : IDisposable
{
    [ThreadStatic]
    private static GCHandle _currentExceptionHandle;
	
    static Activity()
    {
        AppDomain.CurrentDomain.FirstChanceException += (s,e) =>
	{
 	    if (_currentExceptionHandle.IsAllocated)
	    {
	        _currentExceptionHandle.Free();
            }
	    _currentExceptionHandle = GCHandle.Alloc(e.Exception, GCHandleType.Weak);
	};
    }
	
    private readonly IntPtr _exceptionAddress;

    public Activity()
    {
        _exceptionAddress = Marshal.GetExceptionPointers();
    }

    public void Dispose()
    {
        var curentExceptionAddr = Marshal.GetExceptionPointers();
	var isExceptionBubblingUp = _exceptionAddress != curentExceptionAddr && curentExceptionAddr != IntPtr.Zero;
	if (isExceptionBubblingUp && _currentExceptionHandle.Target is Exception currentException)
	{
	    Console.WriteLine($"Thrown exception {currentException}");
	}
	else if (_currentExceptionHandle.IsAllocated)
	{
	    _currentExceptionHandle.Free();
	}
    }
}

@grbell-ms
Copy link
Member

@voroninp I would change GCHandle to WeakReference<Exception> for simplicity, but that's about what I was imagining. ☺️

But I'm no expert, so no promises this works without extensive testing. 😅

@voroninp
Copy link
Author

voroninp commented Aug 2, 2023

I started with Weak reference, but decided, GCHandle will prevent memory allocations.

@adrianotr
Copy link

I ended up with the implementation below. Do you see any possible issues with it?

public sealed class DetailedExceptionProcessor : BaseProcessor<Activity>
{
    private const string EnabledKey = "DetailedExceptionProcessor.Enabled";
    private const string ExceptionKey = "DetailedExceptionProcessor.Exception";

    static DetailedExceptionProcessor()
    {
        AppDomain.CurrentDomain.FirstChanceException += (_, args) =>
        {
            if (Activity.Current is { } activity
                && activity.GetCustomProperty(EnabledKey) is true)
            {
                activity.SetCustomProperty(ExceptionKey, args.Exception);
            }
        };
    }

    public override void OnStart(Activity activity)
    {
        activity.SetCustomProperty(EnabledKey, true);
    }

    public override void OnEnd(Activity activity)
    {
        if (activity.GetCustomProperty(ExceptionKey) is Exception exception 
            && Marshal.GetExceptionPointers() != IntPtr.Zero)
        {
            activity.SetStatus(Status.Error);
            activity.RecordException(exception);
            activity.SetStatus(ActivityStatusCode.Error);
        }
        
        activity.SetCustomProperty(EnabledKey, null);
        activity.SetCustomProperty(ExceptionKey, null);
    }
}

@voroninp
Copy link
Author

voroninp commented May 4, 2024

@jkotas
Copy link
Member

jkotas commented May 4, 2024

Yep came up to similar one https://gist.github.com/voroninp/704c1cb556bd03697ef0c80524de1b24

This will leak the GCHandles over time as threads come and go. There is nothing that would take care of cleaning up of the GCHandles - GCHandles are low-level "unmanaged" resources that must be freed manually.

@voroninp
Copy link
Author

voroninp commented May 4, 2024

@jkotas , despite it allocation with weak type?!

@jkotas
Copy link
Member

jkotas commented May 4, 2024

I ended up with the implementation below. Do you see any possible issues with it?

This will capture the last thrown exception. It won't be the exception that the activity crashed with if there are exceptions thrown and handled during cleanup (finally blocks, using statements in C#, etc.). There is no good way to this 100% right today. It is what this new API proposal is about - introduce an API that allow you to do it properly.

@jkotas
Copy link
Member

jkotas commented May 4, 2024

@jkotas , despite it allocation with weak type?!

Yes, the weak type for GCHandle means that the target is referenced weekly. It does change that the GCHandle itself must be explicitly freed.

You want to use WeakReference<T> here. WeakReference is a GCHandle with finalizer that cleans it up when no longer used.

@voroninp
Copy link
Author

voroninp commented May 4, 2024

@jkotas , thanks, I'll fix it.

@voroninp

This comment was marked as off-topic.

@jkotas

This comment was marked as off-topic.

@voroninp

This comment was marked as off-topic.

@tannergooding tannergooding added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jun 24, 2024
@stephentoub stephentoub added this to the Future milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

7 participants