Add ExceptionDispatchInfo.SetCurrentStackTrace #27004
Conversation
|
FYI, @benaadams, since I know you're interested in stack traces for async stuff. |
We can always relax it later if we find that there are cases where this is called on exceptions that have stacktrace already. I think what's in the PR is fine. |
|
Aside: I also see I would hope that always inlined at R2R & Tier1 as its essentially just a null check; which could be elided in many cases. For example if (context == null)
{
OnCompleted(this);
}
else
{
ExecutionContext.Run(context, s_executionCallback, this);
}So the context has already been tested for null prior to calling EC.Run and the test and throw would hopefully be branch eliminated if it inlines. |
Yes, this code was only executed once, so it wouldn't have been promoted. If I disable tiering, the stack becomes: |
stephentoub
force-pushed
the
setcurrentstacktrace
branch
2 times, most recently
from
c7a54a5
to
d4a9114
Compare
| /// true to throw if the exception already has a stack; false to nop if the exception already has a stack. | ||
| /// </param> | ||
| [StackTraceHidden] | ||
| internal void SetCurrentStackTrace(bool throwIfHasExistingStack) => SetCurrentStackTraceCore(throwIfHasExistingStack); |
There was a problem hiding this comment.
I do not think we need this wrapper. internal void SetCurrentStackTrace(...) can be directly in Exception.CoreCLR.cs.
There was a problem hiding this comment.
partial methods have to be private. What would you recommend instead?
There was a problem hiding this comment.
Delete SetCurrentStackTrace from here. Rename SetCurrentStackTraceCore to SetCurrentStackTrace in Exception.CoreCLR.cs.
There was a problem hiding this comment.
Sorry, I'm not following. SetCurrentStackTraceCore is partial; it has to be private. And as private, it can't be called from ExceptionDispatchInfo. If I make it not partial, then I can't specialize it for coreclr and would instead need another method in Exception.CoreCLR.cs it can call, along with an equivalent method in other partials in other runtimes. If I leave it in Exception.CoreCLR.cs as non-partial, then I'll similarly need to add that same method to other partial files in other runtimes. Am I misunderstanding? Or you're suggesting we do the latter, adding that same method?
There was a problem hiding this comment.
I'll similarly need to add that same method to other partial files in other runtimes
Yes, I think other runtimes should implement this method as well (it is ok to have a empty impl with TODO).
stephentoub
force-pushed
the
setcurrentstacktrace
branch
from
d4a9114
to
3bcb873
Compare
|
Conflicts... |
stephentoub
force-pushed
the
setcurrentstacktrace
branch
from
3bcb873
to
e4042d9
Compare
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Add ExceptionDispatchInfo.SetCurrentStackTrace * Address PR feedback
Contributes to https://github.com/dotnet/corefx/issues/19416
Two open questions from me as I was doing this tonight:
cc: @jkotas
Here's an example of how this manifest. This C# program:
On .NET Core 3.0, it outputs this:
With this API and the corresponding changes in corefx, it now results in:
which provides a lot more details about why this happened.