microsoft / node-api-dotnet

Advanced interoperability between .NET and JavaScript in the same process.
MIT License
495 stars 53 forks source link

NAOT: nullable and exceptions repeated calls #292

Closed daniacedue closed 4 months ago

daniacedue commented 4 months ago

Greetings,

I have found a bug which seems to originate from the code generator. To replicate the bug we need a class which has a nullable property to another class and another method which throws an exception (which we catch in Node JS). Looping through these calls will corrupt something in the generated code somehow.

Here the source code of an example:

using Microsoft.JavaScript.NodeApi;
namespace ExceptionTest
{
    [JSExport]
    public class ExceptionGreeter
    {
        public ExceptionGreeter()
        {
            SubGreeter = new SubGreeter();
        }
        public SubGreeter? SubGreeter { get; set; }
    }
    [JSExport]
    public class SubGreeter
    {
        public void ThrowException(string msg)
        {
            throw new Exception(msg);
        }
    }
}

and on the Node JS side (using typescript):

import {ExceptionGreeter} from "../ExceptionTest/publish-debug/ExceptionTest.js";
const exgr = new ExceptionGreeter();
for (let i = 0; i<100; i++){
    try{
        exgr.subGreeter?.throwException('exception ' + i);
    }catch(err){
        console.log(err);
    }
}

In this example the loop will not reach 100, on my machine it will stop at exactly 54 loops each time ending with an unhandled exception:

Unhandled Exception: Microsoft.JavaScript.NodeApi.JSException: Invalid argument: Error in DefineProperties at D:\a\1\s\src\NodeApi\JSValue.cs:535
   at file:///C:/REPOS3/microsof-nodeapi.tests/ExceptionTest/nodeapp/main.js:5:14
   at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
   at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
   at async loadESM (node:internal/process/esm_loader:28:7)
   at async handleMainPromise (node:internal/modules/run_main:120:12)
   at Microsoft.JavaScript.NodeApi.NodeApiStatusExtensions.ThrowIfFailed(JSRuntime.napi_status, String, String, Int32) + 0x13c
   at Microsoft.JavaScript.NodeApi.JSValue.<>c__DisplayClass108_0.<DefineProperties>b__0(String _, ReadOnlySpan`1 descriptorsPtr) + 0x95
   at Microsoft.JavaScript.NodeApi.JSValue.ToUnmanagedPropertyDescriptors(String, IReadOnlyCollection`1, JSValue.UseUnmanagedDescriptors) + 0x9c0
   at Microsoft.JavaScript.NodeApi.JSValue.DefineProperties(JSPropertyDescriptor[]) + 0xc0
   at Microsoft.JavaScript.NodeApi.JSError.ThrowError(Exception) + 0x80e
   at Microsoft.JavaScript.NodeApi.JSValue.InvokeCallback[TDescriptor](JSRuntime.napi_env, JSRuntime.napi_callback_info, JSValueScopeType, Func`2) + 0x32b
   at Microsoft.JavaScript.NodeApi.JSValue.InvokeJSGetter(JSRuntime.napi_env, JSRuntime.napi_callback_info) + 0x107

If we do other operations like add another method call then it will crash even sooner so it seems to be time-based (perhaps there is an unhandled exception from the first try-catch which hangs on the stack for some time ?)

As a workaround I had to cache the nullable call like this:

const subCache = exgr.subGreeter;
for (let i = 0; i<100; i++){
    try{
        subCache?.throwException('exception ' + i);
    }catch(err){
        console.log(err);
    }
}

this will get to 100 loops.

Thanks in advance.

daniacedue commented 4 months ago

Forgot to mention I was using ES6 modules, I have attatched the config files package.json tsconfig.json

daniacedue commented 4 months ago

Just to give a heads-up: If we change the code to remove the nullable property an use a normal one, it still crashes. So it seems that nullable has little to do with this issue, it is the try-catch of the exception (it is still strange though, why did it work if we cached the nullable previously?)

jasongin commented 4 months ago

Thanks for the bug report. From the description I don't know what's going wrong. I will try to reproduce and debug it.