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

Report the full .NET exception stack trace in fatal errors #386

Open
jasongin opened this issue Oct 1, 2024 · 3 comments
Open

Report the full .NET exception stack trace in fatal errors #386

jasongin opened this issue Oct 1, 2024 · 3 comments
Assignees
Milestone

Comments

@jasongin
Copy link
Member

jasongin commented Oct 1, 2024

There are a couple places in JSThreadSafeFunction that call JSError.Fatal(ex.Message), where the exception message alone might not be enough information to diagnose the error.

As an example, a particular AOT trimming problem resulted in an error:

FATAL ERROR: DefaultCallJS at D:\a\1\s\src\NodeApi\Interop\JSThreadSafeFunction.cs:303
Constructor on type 'Microsoft.Extensions.Http.Resilience.Routing.Internal.RequestRoutingOptions' not found.

The full stack trace revealed that type instantiation was attempted by Activator.CreateInstance() which clearly indicates a trimming issue because the trimmer does not detect such dynamic instantiations.

Either the fatal error message should include the exception stack trace, or we should find a way to propagate the exception as a non-fatal JS error, where the .NET stack will be merged with the JS stack.

@jasongin
Copy link
Member Author

jasongin commented Oct 9, 2024

or we should find a way to propagate the exception as a non-fatal JS error, where the .NET stack will be merged with the JS stack.

The reason this didn't work is because the .NET method was async void. After changing the method to async Task and awaiting it from JS, I got the full combined .NET + JS stack trace.

Still I guess it would be helpful to print the full .NET stack trace along with the fatal error message. Possibly with a note to the effect of "don't use async void".

@jasongin jasongin self-assigned this Oct 10, 2024
@crhaglun
Copy link

This sounds like something that would have been caught by VSTHRD100 - should this project recommend downstream projects to use the VS threading analyzers?

@jasongin
Copy link
Member Author

jasongin commented Oct 17, 2024

I don't think the VS threading analyzers are particularly more important here than anywhere else.

I looked into this more, and there is still a bug here in how the async void exception is handled. Rather than causing an immediate fatal error, it should be passed to Node.js which treats it as an unobserved promise error. I have some pending changes that accomplish that. With those changes then I get this message:

(node:37500) [DEP0168] DeprecationWarning: Uncaught N-API callback exception detected, please run node with option --force-node-api-uncaught-exceptions-policy=true to handle those exceptions properly.

Then if I do run with node --force-node-api-uncaught-exceptions-policy=true, I see the exception message and stack trace. Since this is a deprecation warning, presumably that will be the default behavior in some future version of Node.js.

@jasongin jasongin added this to the Version 1.0 milestone Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants