Allow setting the stack trace on deserialized exception object

[danmoseley]

Proposal

Provide a setter property for SourceExceptionStackTrace in ExceptionDispatchInfo class

namespace System.Runtime.ExceptionServices
{
    public sealed class ExceptionDispatchInfo
    {
        public string SourceExceptionStackTrace { get; set; }
    }
}

Details

Exception serialization scenario is challenging as there is no easy way can deserialize an exception object and include the stack trace in the de-serialized object. The reason is Exception lack supporting a StackTrace setter property or providing a constructor that can take stack trace as a paramter. The net core supports binary serialization which can serialize and de-serialize the exception objects including the stack trace but this approach is not helping much for the following reasons:

• Net core is not promising supporting binary serialization across different net core versions (or with other targets like full framework).
• Other serialization engines (e.g. DateContractSerialization) will not benefit from the binary serialization and need to have a way to fully deserialize exception objects.
• When binary deserialize exception object which include a stack trace and then throw this de-serialized object, the stack trace after throwing is the one reflect the place where we threw this exception but the original de-serialized stack trace will be lost. The original stack trace will be preserved only in case re-throw previously thrown exception. The following code demonstrate this issue

            try
            {
                throw new Exception();
            }
            catch (Exception e)
            {
                using (MemoryStream ms = new MemoryStream(100))
                {
                    BinaryFormatter formatter = new BinaryFormatter();
                    formatter.Serialize(ms, e);

                    // reset the stream
                    ms.Position = 0;

                    var excp = (Exception)formatter.Deserialize(ms);

                    Console.WriteLine(excp.StackTrace); // this will show the right original serialized stack 

                    try { throw excp; } catch (Exception excp1) { Console.WriteLine(excp1.StackTrace);  } // this will show the new stack but the old de-serialized stack will be lost.

                    throw; // the thrown exception will include the original exception stack in “e” object 
                }
            }

To support the scenario, we need to add SourceExceptionStackTrace property to ExceptionDispatchInfo class. The following code demonstrate how ExceptionDispatchInfo can be used to get the exception with the original serialized stack trace and also how to throw the exception with the new stack trace.

            ExceptionDispatchInfo edi = ExceptionDispatchInfo.Capture(deserializedException);
            edi.SourceExceptionStackTrace = originalStackTrace;
            Exception deserializedExceptionWithOriginalStackTrace = edi.SourceException;
            edi.Throw();

Behavior ExceptionDispatchInfo.SourceExceptionStackTrace

The setter of this property will overwrite the current source trace on the source exception. the setter will not affect any other properties in the source exception object.

The getter will return the stack trace from the source exception. this can be empty string if there is no stack trace in the source #exception

Alternative Design

We can think in adding a setter for Exception.StackTrace property or expose a method like Exception AddPreservedStack(string stackTrace))

namespace System
{
    public class Exception : ISerializable, _Exception
    {
        public virtual String StackTrace { get; set; }
    }
}

I got some feedback from different people that they don't like this option as in the main scenarios nobody should set the stack trace on the exception objects and this option can cause confusion and people misuse it.

[tarekgh]

This issue is broken out from #17144, see context there.

CC all people who commented on the original issue #17144
@KrzysztofCwalina @eiriktsarpalis @terrajobst @SGuyGe @niemyjski @danmosemsft @stephentoub @jkotas @weshaggard

Having Exception.StackTrace setter was requested before we had the serialization constructors exposed on the Exception class. now we have the serialization constructors which can be used to deserialize the exception object. StackTrace setter is not really needed now except if we need to enable serialization scenarios which not using the serialization constructors. I can see exposing it will be useful for such serialization scenario but it will be interesting for non-serialization scenario to allow having a setter for exception stack trace. and it is not clear either if we need to support such serialization scenarios, the Exception class will not be only type will have such issue but I expect many other types will have similar issue.

what all think about that? do you still think we need to expose this property?

[danmoseley]

Removing the label since as you say this is no longer required for our NS2.0 goal. It seems to me this is a feature at this point and not something that needs doing right now.

[eiriktsarpalis]

My opinion has always been that making the stacktrace settable is not a
very good idea
On Tue, 15 Nov 2016 at 01:22, Dan Moseley notifications@github.com wrote:

Removing the label since as you say this is no longer required for our
NS2.0 goal. It seems to me this is a feature at this point and not
something that needs doing right now.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/corefx/issues/9294#issuecomment-260518280, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACrtsz-lNRCk06qpuGd5eJigSEiCEJZQks5q-QlCgaJpZM4IyQLU
.

[eiriktsarpalis]

I'm happy that the constructors are back. ISerializable is not the best
abstraction, but at least it lets us send exceptions over the wire.
On Tue, 15 Nov 2016 at 01:54, Eirik George Tsarpalis <
eirik.tsarpalis@gmail.com> wrote:

My opinion has always been that making the stacktrace settable is not a
very good idea
On Tue, 15 Nov 2016 at 01:22, Dan Moseley notifications@github.com
wrote:

Removing the label since as you say this is no longer required for our
NS2.0 goal. It seems to me this is a feature at this point and not
something that needs doing right now.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/corefx/issues/9294#issuecomment-260518280, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACrtsz-lNRCk06qpuGd5eJigSEiCEJZQks5q-QlCgaJpZM4IyQLU
.

[tarekgh]

Thanks @eiriktsarpalis.
I am going to close this request for now and we can open it again if we find any case really need this setter.

[niemyjski]

I have seen people set a custom stack trace... I agree it should be gone, it will break some people tho.

[jkotas]

@tarekgh The current plan is not support binary serialization accross different versions of the runtime/framework. The solution based on binary serialization assumes that there is same version of the runtime/framework running on all machines. I do not think it is safe to make this assumption.

[tarekgh]

CC @weshaggard

[joperezr]

@tarekgh Is this something we have decided to take?

[tarekgh]

@joperezr Yes I am going to work on the proposal next week and then will mark it as ready for review.

[tarekgh]

@jkotas I have added the proposal on the top of this issue, could you please have a look and send your feedback? I already marked the issue ready for design review.

[jkotas]

Have you considered exposing it on ExceptionDispatchInfo as a design alternative? It would make the setter to be less in your face.

@gkhanna79 Any comments about this API proposal?

[eiriktsarpalis]

@jkotas Agree 100% with moving this to ExceptionDispatchInfo. Modifying stacktraces should only be made available to library/framework authors, and thus hidden from common view.

[304NotModified]

Modifying stacktraces should only be made available to library/framework authors, and thus hidden from common view.

Agree also on that.

[tarekgh]

@jkotas it looks having it on ExceptionDispatchInfo could be better choice. thanks for pointing to that.
I'll change the proposal to use ExceptionDispatchInfo and I'll make my current proposal as the alternative design

[gkhanna79]

I concur - ExceptionDispatchInfo is way more appropriate for this task and does have the notion of "propagating" stacktraces that can be leveraged to address this scenario.

[tarekgh]

I have updated the proposal according to the feedback. thanks all for your feedback.

[terrajobst]

I think not exposing a setter for the stack trace on Exception makes sense. What's the behavior of ExceptionDispatchInfo.SourceExceptionStackTrace? Is this always a set or is it an append? If it's an append than it shouldn't be a property but a method. We could also make this a method that offers both variants. The reason being that

var edi = ExceptionDispatchInfo.Capture(x);
edi.Throw();

will not preseve the original stack trace but will append the new stack trace to preserve diagnostic information.

[tarekgh]

@terrajobst in the current proposal in the behavior section, I proposed to overwrite the stack trace and not to append it. The reason is we are addressing the serialization scenario which doesn't need the append here. also with the current proposal, people still can append their stack if they need to just by getting the stack trace then append whatever they like and then set it back.

[304NotModified]

Set also sounds more logical than append IMO

[ViktorHofer]

@danmosemsft do you believe this is still necessary?

[danmoseley]

I don't see the need given we have serialization. Maybe close and see whether we get feedback.

[YairHalberstadt]

I am using a grpc interceptor to catch all exceptions thrown on the server side, serialise them, intercept on the client side, catch the exception and rethrow it.

The problem with this approach is that either I only get the stack trace on the server end, or I only get the stack trace on the client end. Neither is sufficient to diagnose why the exception was thrown.

I need a way to merge the stack traces, and this proposal looks like it would provide such a capability.

[danmoseley]

@YairHalberstadt can you wrap the exception on the client side and rethrow that?

[YairHalberstadt]

@danmosemsft

That's what I'm doing, but a key purpose of serialising the exception is to be able to differentiate in catch clauses on the type of exception.

Now we have to catch our wrapper exception and switch on the inner exception type.

Certainly not the worst thing in the world, but I think this presents a valid use case for this API.

[tarekgh]

I think using inner exception would be reasonable here, at least you'll have 2 exception objects instead of merging stack trace. The other similar idea is to use AggregateException and wrap all exceptions inside one object?

[amanbha]

Is this being considered? Throwing an AggregateException will violate the promise of await as await should not throw AggrgateException. So throwing my new exception which has the the actual server info as inner exception is the only option.