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: ExceptionDispatchInfo.TrySetStackTrace #50044

Closed
GrabYourPitchforks opened this issue Mar 22, 2021 · 8 comments · Fixed by #50392
Closed

API Proposal: ExceptionDispatchInfo.TrySetStackTrace #50044

GrabYourPitchforks opened this issue Mar 22, 2021 · 8 comments · Fixed by #50392
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Mar 22, 2021

Scenario

One of the primary uses of BinaryFormatter is to serialize Exception objects. Since we are obsoleting this serializer, we need to investigate new ways of serializing exception data.

For most Exception-derived types, serializers can be written to call the applicable public ctor based on parameter name (for ArgumentException), HRESULT, exception text, and inner exception object. However, one big omission is the ability to set the stack trace on a newly-constructed, unthrown Exception instance. Without this, "safe" Exception serializers and deserializers cannot reproduce the initial exception object with a high enough fidelity to allow for debugging.

Proposed API

namespace System.Runtime.ExceptionServices
{
    public sealed class ExceptionDispatchInfo
    {
        // new proposed API
        public static bool TrySetStackTraceString(Exception source, string stackTrace);

        // existing relevant public API, for reference (no changes proposed)
        public static Exception SetCurrentStackTrace(Exception source);
    }
}

Usage

// Assume deserializer is attempting to reconstruct an InvalidOperationException instance
// from a serialized JSON payload.

var jsonObject = GetJsonObject();

string? message = jsonObject["message"]?.Value;
Exception? innerException = DeserializeExceptionRecursive(jsonObject["innerException"]); // up to some recursive limit
string? stackTrace = jsonObject["stackTrace"]?.Value;
int? hr = jsonObject["hr"]?.IntValue;

var reconstructed = new InvalidOperationException(message, innerException);
if (stackTrace != null) { ExceptionDispatchInfo.TrySetStackTraceString(reconstructed, stackTrace); }
if (hr != null) { reconstructed.HResult = (int)hr; }

// Now you can return the exception object as-is, as a raw deserializer would do
return reconstructed;

// Or you can throw it, as an RPC proxy would do
ExceptionDispatchInfo.Capture(reconstructed).Throw();

Behavior and discussion

The method requires non-null Exception and string arguments. If the exception instance has been previously thrown or already has an associated stack trace, the method has no effect and returns false. (ExceptionDispatchInfo.SetCurrentStackTrace would throw in this scenario.) If the exception instance is an immutable or singleton exception, the method has no effect and returns false. (ExceptionDispatchInfo.SetCurrentStackTrace would no-op in this scenario.) Otherwise the Exception.StackTrace property is set to the provided string value and this method returns true.

No validation is performed over the contents or shape of the provided string. It is completely opaque to the runtime.

The Exception.TargetSite property will remain null after the stack trace string has been set. The runtime will not attempt to parse the stack trace string and recover the original MethodBase object. Additionally, the exception object's Watson bucket data will remain unpopulated. This behavior is to support the ideas that: (a) the stack trace might be provided by an untrustworthy source and thus must not be run through the reflection APIs; and (b) the stack trace might refer to an assembly not available to the current process.

Alternative designs

We could require capturing the exception within the EDI before allowing setting the stack trace string. This may even allow us to overwrite the stack trace string from a previously-thrown exception. Doing this would also promote the idea of "this is meant for the purpose of restoring a stack trace from ancillary data before re-throwing the exception" as a first-class concept, even above vanilla deserialization. (The deserialization-without-rethrowing scenario would still be achieveable; simply check the ExceptionDispatchInfo.SourceException property instead of rethrowing it.)

However, the existing SetCurrentStackTrace method does not require an EDI wrapper, so for symmetry the main proposal above also does not require the existence of an EDI wrapper.

We could also consider adding a method SetStackTrace directly to the Exception object. However, I believe this will encourage misuse of what is supposed to be a specialized API. Putting this new API on the EDI class makes it a proper sibling of the existing SetCurrentStackTrace API, which further helps symmetry.

>> Marked partner blocking (/cc @ReubenBond)

@GrabYourPitchforks GrabYourPitchforks added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work labels Mar 22, 2021
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone Mar 22, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 22, 2021
@jkotas
Copy link
Member

jkotas commented Mar 22, 2021

If the exception instance has been previously thrown or already has an associated stack trace, the method has no effect and returns false.

Where do we expect this to be called on exceptions that have a stacktrace set already? Why not throw like what SetCurrentStackTrace does?

@pgovind pgovind added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 22, 2021
@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Mar 22, 2021

@jkotas I don't feel strongly one way or the other on this. If there's a way for a caller to detect likely failure ahead of time (query Exception.StackTrace and see if it's populated?), then the try pattern doesn't add much.

@stephentoub
Copy link
Member

Just to make sure I understand the proposed semantics, SetCurrentStackTrace would then be logically equivalent to:

return SetStackTraceString(exception, GetCurrentStackTrace());

?

@GrabYourPitchforks
Copy link
Member Author

Just to make sure I understand the proposed semantics, SetCurrentStackTrace would then be logically equivalent to SetStackTraceString(...)?

Basically, yeah. I suppose SetCurrentStackTrace could also in some future release modify Exception._stackTrace (the binary blob) if there were some scenario where that's useful. That's a capability that SetStackTraceString would never have.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Mar 23, 2021
@terrajobst
Copy link
Member

terrajobst commented Mar 23, 2021

Video

  • Let's make this a non-try method (and let it fail if it can't work)
  • We should make sure that new StackTrace(exception) doesn't blow up when someone catches the exception and tries to parse the stack trace
namespace System.Runtime.ExceptionServices
{
    public sealed class ExceptionDispatchInfo
    {
        public static void SetRemoteStackTrace(Exception source, string stackTrace);

        // existing API for reference
        // public static Exception SetCurrentStackTrace(Exception source);
    }
}

Edit I've replaced bool with void as that was a typo.

@GrabYourPitchforks
Copy link
Member Author

I verified the "arbitrary string" behavior in two ways.

First, I called SetRemoteStackTrace(ex, "😺") and verified that the results of Exception.StackTrace and Exception.ToString() are sensible.

  Name Value Type
  ex.StackTrace "😺\r\n--- End of stack trace from previous location ---\r\n at ConsoleApp60.Program.DoSomething()" string
  ex.ToString() "System.Exception: Hello!\r\n😺\r\n--- End of stack trace from previous location ---\r\n at ConsoleApp60.Program.DoSomething()" string

(The DoSomething method is where the reconstructed exception object was thrown after setting the stack trace string.)

I also verified the behavior of new StackTrace(Exception). This API only returns the frames that were captured as part of the most recent throw statement. It never includes any remote stack trace information. And if the Exception instance has not yet been thrown, new StackTrace(Exception) returns an empty set.

As far as I can tell this means we should be good to go.

@terrajobst
Copy link
Member

But does SetRemoteStackTrace(ex, "🦦") also work?

@ReubenBond
Copy link
Member

But does SetRemoteStackTrace(ex, "🦦") also work?

Not for the InnerException, but it is supported by-design on the OtterException

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 29, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants